-
Notifications
You must be signed in to change notification settings - Fork 164
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
Conversation
There was a problem hiding this 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"
@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. |
text/0005-global-init.md
Outdated
`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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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. |
@SergeyKanzhelev I added a couple of paragraphs of further motivation. |
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? |
@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. |
text/0005-global-init.md
Outdated
subsequent calls to the explicit initializer shall log console | ||
warnings. | ||
`Meter` factories (e.g., `opentelemetry.Init(TracerFactory, | ||
MeterFactory)`), factories because they construct explicitly named |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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. |
@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 |
Yes, I'd expect to be some code in main() that instantiates OTel and wraps it in OpenTracing bridge.
If you're talking about agent-based "zero touch" instrumentation, then it can always do the global magic behind the scene, can it not?
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. |
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:
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:
(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.) |
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. |
This PR is stale. I will close it and re-open a new PR with a revision to this OTEP. |
See #74. |
No description provided.