Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to 0005 Global Initialization #45

Closed
wants to merge 8 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Sep 11, 2019

No description provided.

Copy link
Member

@iredelmeier iredelmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for getting 0005 accepted per #44, right? Might make sense to update the title to something like "WIP: Accept RFC 0005"

text/0005-global-init.md Outdated Show resolved Hide resolved
text/0005-global-init.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Sep 12, 2019

@iredelmeier I thought the point of #44 was to open a WIP PR for discussion to take place, since I don't really think is accepted.

@jmacd jmacd closed this Sep 13, 2019
@jmacd jmacd deleted the jmacd/globalinit branch September 13, 2019 18:15
@jmacd jmacd restored the jmacd/globalinit branch September 13, 2019 18:16
@jmacd jmacd reopened this Sep 17, 2019
@tedsuo tedsuo added this to the Alpha v0.3 milestone Oct 8, 2019
`Meter` factories (e.g., `opentelemetry.Init(TracerFactory,
MeterFactory)`), factories because they facility explicitly named
`Tracer` and `Meter` instances. The SDK must not be installed more
than once. After the first SDK installed, subsequent calls to the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree that resetting the globals should be discouraged, completely restricting it feels unnecessary to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about the alleged risk? If anyone can install an SDK whenever they want, how do you (a main() function) prevent others from installing SDKs when you know what you want?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my worry is how multi-host processes will deal with the global tracer? If one process hosts two apps, each of them maintainig it's dependency injection and creates own tracerfactory. For the libraries that needs to use "global" tracer, which do not use DI, second app can register itself somehow in the global tracer later.

It may be C# specific, but I can see how it also may be a case in Java.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This OTEP is about how the globals are initialized, not about whether or not to support globals. I believe that debate has been had 100 times and globals are going to stay. That said, I'm trying to restrict them as much as possible by allowing them to be initialized once. Applications that wish to use multiple SDKs should avoid the globals and use DI. If there are libraries that use the globals, then there will be a problem in this scenario--either don't use globals or do use globals, they don't mix well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, I'm not entering that debate =). In C# and .NET Core this problem is in many places. The best practice is to not use globals and use dependency injection (DI) instead. However there are still plenty of libraries that uses globals. So when you are registering yourself in DI, you also need to register in global.

The problem occurs when a single process is shared by two apps. Each of them use DI for all libraries that adopter DI. For libraries relying on static globals - ideally - should be a way to decide at call site which tracer to use. And each of those two apps can register themselves in that global tracer for awareness.

So in this situation there is a case of later update of a global tracer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question is not about the debate, but about clearly documenting the need and reasons for supporting globals in OTel.

Also, iirc, when we had this debate in the spring, we agreed that any documentation / facilities for globals should come with a disclaimer DO NOT USE THIS.

text/0005-global-init.md Outdated Show resolved Hide resolved
@SergeyKanzhelev
Copy link
Member

@jmacd looking at comments I feel that the lack of response is caused by unclear motivation. Perhaps you can add a few sentences in the proposal with the reasoning.

Also I left comment about multi-host apps. Given I haven't thought about it extensively, perhaps you can add your thoughts or add a sentence about this in the document.

@jmacd
Copy link
Contributor Author

jmacd commented Oct 16, 2019

@SergeyKanzhelev I added a couple of paragraphs of further motivation.

@yurishkuro
Copy link
Member

At the risk of sounding like a broken record, but the best way to avoid confusion about how to initialize globals is to avoid damned globals. Is there any single write-up in the spec or oteps that illustrates, unequivocally, a scenario that cannot be solved without globals?

@jmacd
Copy link
Contributor Author

jmacd commented Oct 22, 2019

@yurishkuro I believe "OpenTracing compatibility" is a sufficient motivation.

No one argues that there are situations that are un-solvable without globals. The argument is that for a large or legacy code base, to add instrumentation and not add a bunch of new dependency injection-- especially in languages that don't automate DI--that it is an unwelcome burden to force them to pass new providers throughout their code. Globals make testing harder, sure, but in these code bases, it is unlikely that developers are going to write tests for their instrumentation. As they begin to want tests, they will migrate away from the globals. Globals help incrementally adopt a new instrumentation API.

We should not be in the business of telling people how to write and test their code. If I want to quickly add instrumentation to an existing code base, globals are the way to go.

subsequent calls to the explicit initializer shall log console
warnings.
`Meter` factories (e.g., `opentelemetry.Init(TracerFactory,
MeterFactory)`), factories because they construct explicitly named
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there would be benefit to split this into two calls (being able to initialize the tracer and meter independently). Any particular reason to keep these tied together in the same api call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel very strongly about this. I was imagining that it's important for an application to know that their metrics and traces began coming through at the same moment in the process lifetime.

@yurishkuro
Copy link
Member

@jmacd can you elaborate why "OpenTracing compatibility" requires globals in OTel?

Why can't this be the problem of those legacy applications? Declaring a global is not difficult, they can always do it if they must, and they have much better understanding of their start-up sequence to ensure what the initialization order if appropriate (instead of us trying to guess how it must work in a generic framework). But if we make globals a part of the official API, and have bunch of examples that rely on it, then people will start writing instrumentation that is coupled with that global API, often without providing for proper DI.

@jmacd
Copy link
Contributor Author

jmacd commented Oct 22, 2019

@yurishkuro I'm thinking of the OpenTracing bridge, which is supposed to provide compatibility for code that is incrementally upgraded to OpenTelemetry. That code may use the OpenTracing global tracer. What is the bridge to do, if there is no OpenTelemetry global tracer? I can see an argument that this will be configured as the bridge is set up, without requiring an OpenTelemetry global.

You suggested that a body of code that wants to incrementally migrate as I described could instead use its own global. This will require some kind of code modification (e.g., a new SetMyCodeGlobalTracer()). Callers of this body of code will have to be modified to inject that tracer. It does not achieve the goal "zero touch" instrumentation. If I depend on a body of code, and that body of code begins using OpenTelemetry instrumentation, I should not have to change my code at all. That requires a global, I think.

@yurishkuro
Copy link
Member

What is the bridge to do, if there is no OpenTelemetry global tracer? I can see an argument that this will be configured as the bridge is set up, without requiring an OpenTelemetry global.

Yes, I'd expect to be some code in main() that instantiates OTel and wraps it in OpenTracing bridge.

It does not achieve the goal "zero touch" instrumentation.

If you're talking about agent-based "zero touch" instrumentation, then it can always do the global magic behind the scene, can it not?

If I depend on a body of code, and that body of code begins using OpenTelemetry instrumentation, I should not have to change my code at all.

So an example would be some RPC framework that in v1 did not support OTel, but in v2 it does, and you want to simply bump the legacy application's dependency to RPC v2 and do nothing else? How realistic is that, again assuming that you're not doing agent-based instrumentation? I think this becomes very language specific.

I would concede that if there are many dependencies like that, you'd need some shared global mechanism (because one dependency can always have a custom module for global initialization). But I am positive that as soon as we make it officially available, the instrumentations will start popping up that depend on it, whether they need it or not.

@jmacd
Copy link
Contributor Author

jmacd commented Oct 22, 2019

I do expect this approach to work without any sort of agent-based auto-instrumentation. If you link in a library as a result of some dependency chain, and that library uses the global tracer, you get automatic instrumentation.

This OTEP was filed to clarify the situation with respect to globals during initialization. Globals were already in the spec. The tracing API says:

Tracers are generally expected to be used as singletons. Implementations
SHOULD provide a single global default Tracer.

In the recently-merged metrics API, we made changes that helped avoid the need for a global registry of metrics. Prior to the change, I had been trying to make metric instruments be things you could declare statically, which is a common pattern. The change says that metrics are registered via the Meter, which has pros and cons. The pros are that you don't need a global registry, metrics are automatically namespaced by their Meter, and metrics can be exported prior to first use. The cons are that you can't allocate your metric instruments statically, without resorting to a global. The reviewers of the metrics spec suggested we simply recommend use of the global Meter to allocate metrics, which will definitely push people towards use of the global Meter. I'm referring to this language:

We recognize that many existing metric systems support allocating metric instruments statically and providing the Meter interface at the time of use. In this example, typical of statsd clients, existing code may not be structured with a convenient place to store new metric instruments. Where this becomes a burden, it is recommended to use the global meter factory to construct a static named Meter, to construct metric instruments.

The situation is similar for users of Prometheus clients, where instruments are allocated statically and there is an implicit global. Such code may not have access to the appropriate Meter where instruments are defined. Where this becomes a burden, it is recommended to use the global meter factory to construct a static named Meter, to construct metric instruments.

(I'm playing devil's advocate here. I don't want to use the global tracer in my code, but I don't really see it as a problem either. What's the downside--that you can't run parallel tests? That seems like a minor loss compared with the benefit of easy integration. On the other hand, I enjoy being able to allocate metric instruments statically, but the pros outweighed the cons to me and I'm on board with creating a struct to hold all my metric instruments and using a dependency-injected Meter to allocate them.)

@jmacd
Copy link
Contributor Author

jmacd commented Oct 31, 2019

For the record. This was discussed briefly in the 10/29/19 Spec SIG meeting. It was suggested that possible this is a language-specific issue to Go, but I believe the same issue is present in C++ for the same reasons. It may be only these two languages, I'll concede.

@jmacd
Copy link
Contributor Author

jmacd commented Dec 20, 2019

This PR is stale. I will close it and re-open a new PR with a revision to this OTEP.

@jmacd
Copy link
Contributor Author

jmacd commented Dec 20, 2019

See #74.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics Relates to the Metrics API/SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants