Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(instrumentation): add util to execute span customization hook in base class #4663
feat(instrumentation): add util to execute span customization hook in base class #4663
Changes from 10 commits
c55be5e
a0e2a8d
d0cd4d5
1c059fe
c33ba1a
944a224
cac3766
fa1a53e
9dc607c
9f44c05
d29bda6
86fb72a
c2d7518
e737b7f
68c73ba
1540713
9d0daf9
2035a34
73b1308
d4e2f38
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Hmm, I wonder if we can make this whole protected method actually a stand-alone function.
As a protected method it's actually part of the public interface anyway, so adding it as a stand-alone function does turn out to be the same. I'm worried that, as more signals become available, we add a lot of util functions to the instrumentation base that will make it blow up in size over time.
Adding things to abstract classes is also difficult once stable, as in typescript consumers can use the type like:
We've seen this happen with the (non-abstract) Resource class #3676
I feel putting this here now may spark lots of requests later to add more causing us to run into a similar problem (I'll be less impactful than the Resource one, as I don't assume people use it like I described above a lot, but it can still happen and can be breaking).
It's actually something that happens occasionally with
MetricReader
too, especially with@opentelemetry/sdk-node
where@opentelemetry/sdk-metrics
is pinned.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 would love to have it as util, but since it's using
this._diag
I need access tothis
which felt like a class member function.Having it as util would mean passing also the diag logger as an argument to the function, which felt strange to me.
WDYT?
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.
For the code snippet above, I guess it would make more sense to use the
Instrumentation
interface and not type something asInstrumentationBase
directly, although no one will stop users from doing this.Do you think once stable we should avoid adding new methods to
InstrumentationAbstract
, or are we still allowed to do that but should try not to if possible?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.
Hmm, I see, let's leave it like this then and re-evaluate later.
Hard to say. I think we should at least try not to do it. I've tried to find resources on this topic (specifically related to typescript libraries) but I was unsuccessful in finding a concrete answer to the question (which I've had now for quite a while). Most of the resources online and in books are specifically related to TypeScript in apps which is unfortunately not as helpful for my specific questions as a library author.
From what I've seen users expect that these breakages should not occur outside of major versions and, intuitively, I agree. For the
MetricReader
we've done it before simply because that's the only way we could make any changes at all. Impact on end-users is fairly minimal.For TypeScript, it is extremely easy to break users when exporting classes, this is further complicated by having situations that allow for multiple versions of the same package, and the fact that private class members are part of the public interface, too. Adding anything is a nightmare when being strict enough.
What that'd mean for us is that:
I'm not sure if that's right or the the way to go though. Let's leave this PR as-is and discuss this another time. It's likely a larger change that will affect all components and needs to be carefully considered.
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.
Thanks for sharing your thoughts and describing the problem so clearly. I agree, it's best to leave this PR as-is and reevaluate later 🙏
Trying to think how these considerations can be applied in the InstrumentationBase case. would love to discuss this later on
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 was wondering if it can be useful to also always include the
eventName
as an attribute in the event info, something like:and adjust the types.
The hook can always choose to ignore this parameter but it will be available if needed. Not sure thought if it's worth adding more complications to the code for a non existing use that I'm aware of at the moment.
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 would be helpful if a hook function is used for several hooks. I have no usecase in mind. Price is the extra object created.
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 thinking as well. thanks. will wait for more opinions.
We can also add it in the future in a non breaking manner
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.
As part of this PR, I also transformed the diag message to be more structured - make the log message constant and add the eventName as an argument so it is easy to consume.
I believe these type of diag prints are better and should be preferred, but am open to address this in a different PR
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 chose to make the generic type
EventInfoType
required, to promote typings of the object.Since some instrumentations are not able to type the object correctly, which might require type export from the instrumented package itself, it is not always possible. In this case the instrumentation can choose to use
any
in the hook, which communicates that the type was ignored on purposeThere 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.
@open-telemetry/javascript-approvers This PR also introduces a new name: "InstrumentationEvent". At the moment, instrumentation has partially settled on a hook function signature and behaviour. The pattern is a "{some event name}Hook" function that receive a span along with info object, at specific times in the lifecycle of a span it records, and the hook is used to add additional attributes.
I wanted to document it as a type and give it a name that we can use which will promote consistency and reduce mental load for instrumentation writers and maintainers.
WDYT about the name "InstrumentationEvent"? It can only be used in the context of a specific open span (for which we can add attributes), required info object, and no return value.