Naming of span related APIs on context (setExtractedSpanContext(), getParentSpanContext(),...) #1713
Closed
1 of 2 tasks
Labels
Discussion
Issue or PR that needs/is extended discussion.
Comments
|
Noone an opinion regarding this? If we decide to do this we have to do it before GA. |
|
Well i have nothing against, i'm wondering if we should move them to the core package though. Those are mostly utils, having them on the api means we should support them and be careful about breaking changes. |
|
I don't think that moving this to core is a good choice because it would result in plugins to depend on core. e.g. http plugin uses Having all the context topics in API (or context-base) ensures that plugins are useable with 3rd party SDKs. I expect for the same reason plugins are move to instrumentations. |
pichlermarc
pushed a commit
to dynatrace-oss-contrib/opentelemetry-js
that referenced
this issue
Dec 15, 2023
…n-telemetry#1713) * feat(instrumentation-bunyan): add log sending to Logs Bridge API This extends the Bunyan instrumentation to automatically add a Bunyan stream to created loggers that will send log records to the Logs Bridge API: https://opentelemetry.io/docs/specs/otel/logs/bridge-api/ Now that the instrumentation supports separate "injection" of fields and "bridging" of log records functionality, this also adds two boolean options to disable those independently: `enableInjection` and `enableLogsBridge`. This also updates the instrumentation to work with ES module usage. Closes: open-telemetry#1559 * markdown lint fixes * markdown lint fixes * catch up with recent core-deps update * some type tweaks suggested by David * more specific type Co-authored-by: Amir Blum <amirgiraffe@gmail.com> * use more self-explanatory code for mapping Bunyan level to OTel severity, from blumamir * export OpenTelemetryBunyanStream for direct usage in Bunyan loggers without the instrumentation * .apply over .call suggestion * consistency suggestion * suggestion to use the longer (perhaps clearer) logger var name * switch to false-by-default config vars to avoid surprises with undefined values See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#boolean-value Suggestion from blumamir. * document using OpenTelemetryBunyanStream without the instrumentation * fix https://eslint.org/docs/latest/rules/prefer-spread lint error * drop options to OpenTelemetryBunyanStream constructor because YAGNI * temporarily drop CI caching to test theory on unit-test (18) CI failure * more CI debugging: restore cache, add some 'npm ls -a' to look into NoopContextManager being used * elide Bunyan 'pid' and 'hostname' fields in OTel log record attributes Because they are redundant with 'process.pid' and 'host.name' resource attributes. Add some docs on how to use resource detectors to the example, because the HostDetector is not on by default in the NodeSDK. * update test for having elided 'pid' and 'hostname' fields * CI debugging: ignore the 'npm ls -a' exit status, they shouldn't break the build * fix lint and compile errors * CI debugging: turn on diag DEBUG to test a theory * turn off diag in this example * undo CI debugging changes * update deps to current releases and sync package-lock.json * disableInjection -> disableLogCorrelation * disableLogsBridge -> disableLogSending Avoid using 'bridge' terminology at suggestion from specs that the Bridge API is an internal detail. * correct the default instrumentation scope name (as discussed earlier) * tests: fix test for intrumentationScope.name change in previous commit * fix lint --------- Co-authored-by: Amir Blum <amirgiraffe@gmail.com> Co-authored-by: Hector Hernandez <39923391+hectorhdzg@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I find the current naming of span related APIs exported by context.ts quite confusing and misleading:
setActiveSpan()andgetActiveSpan()which are symmetric which is quite ok.setExtractedSpanContext()which actually callssetActiveSpan()and there is nogetExtractedSpanContext()getParentSpanContext()which actually returns theSpanContextofgetActiveSpan()Since #1589 there is no differentiation anymore between an active span, extracted span or parent span - there is just "the span" on context.
I think we should cleanup the API and remove
getParentSpanContext()andsetExtractedSpanContext().And maybe
[gs]etActiveSpan()should be renamed to[gs]etSpan()or[gs]etSpanOnContext().The text was updated successfully, but these errors were encountered: