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
Add tracing suppresing for Metrics Export #3332
Add tracing suppresing for Metrics Export #3332
Changes from 5 commits
b1f6992
c948fa9
555ff46
99c2c78
c0a81d0
f8661d5
86a9c20
3b9b20b
a5d1291
7cf89d5
aeb33e2
799add6
fa08664
9a55db5
b5427ce
be123f1
17948ba
9d92bb1
00614a1
30a1dca
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.
coreExport is sort of an odd name for a function in core I think. Also can you please add tsdocs since it is very non-obvious what this does to an outsider.
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 worry a little bit about adding yet more API surface to the core module. I was hoping to eventually phase out the core module completely since it seems to just be a dumping ground for things. There also are other considerations beyond just suppressing tracing on export. We may for example want to suppress other signals like metrics.
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.
We should have a dedicated module to share common snippets across SDK packages. I find the problem with the @opentelemetry/core is that the APIs that are supposed to be internal are exposed as public interfaces. Would it make sense to you to explicitly document this method as an internal interface so that we don't provide any API stability guarantees? Or should we create a new internal util package?
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 a reasonable compromise to document this as internal/unstable. IMO all of core should have been internal but it's too late for that.
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.
Added internal tsdoc, under score is usually added at the beginning of the name of the function, I'm not sure if you are using this pattern in this repo but let me know if that is a problem https://tsdoc.org/pages/tags/internal/
@dyladan is Metrics signal suppression already defined or implemented somewhere in OpenTelemetry?, trace suppression should make the generated Spans to be "not recording" ones, those should not be considered for metrics, Ex. HTTP duration metrics.
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 think "all" of them are internal. I suppose APIs like
CompositePropagator
are public -- as there is no good place to export them.