-
Notifications
You must be signed in to change notification settings - Fork 812
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
Conversation
experimental/packages/opentelemetry-sdk-metrics/src/export/PeriodicExportingMetricReader.ts
Outdated
Show resolved
Hide resolved
export(arg: T, resultCallback: (result: ExportResult) => void): void; | ||
} | ||
|
||
export function coreExport<T>(exporter: Exporter<T>, arg: T, resultCallback: (result: ExportResult) => void): void { |
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.
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.
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.
experimental/packages/opentelemetry-sdk-metrics/src/export/PeriodicExportingMetricReader.ts
Outdated
Show resolved
Hide resolved
@dyladan @legendecas looks like this one is ready to merge, let me know if there is any concern. Thanks |
No, it's ready to go. Thanks for working on this! |
Fixes #3331