-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Consolidate and remove duplicate tracing JS content #1723
Consolidate and remove duplicate tracing JS content #1723
Conversation
Leaving that to the @open-telemetry/javascript-approvers , but I would find it kind of sad to see some of that content with code examples gone. Maybe we can do this gradually? Remove those sections that are duplicates for sure, and then we can figure out where those other pieces should live:
|
I'm fine with more gradually migrating, although the biggest code sample here is basically showing how to create child spans in the lowest-level way possible, which we probably shouldn't encourage. I'd rather have people land on the manual instrumentation page. Regarding this:
Do you mean the fact that the code samples used the typescript language in the markdown? In theory yeah, we should have typescript samples for everything. But with this still open there can be considerable problems (FWIW I would prefer to standardize on ESM for everything otel-js, as that's what editor tooling asks you to do as wel). |
Fair point, then let's whait for the JS SIG to comment.
No, I mean in the overall concepts section we could have code snippets in the major languages of otel (Java, .NET, Go, JS, etc.) in a tabbed example (like erlang/elixier does it for all examples) |
@open-telemetry/javascript-approvers any thoughts on this PR? |
I agree that it may be preferable to move this documentation in a more specific doc but i'm not sure where it is right now ? (A bit linked to open-telemetry/opentelemetry-js#3255) |
@vmarchaud Yeah, that's why I wanted to get rid of the section. It says "API", but it's not an API reference, and what it actually shows right now is just a different way to accomplish what's already documented under /instrumentation. |
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.
+1 on this change. It reduces the duplicity between the language document and the common concept document. 👍
@cartermp Ignore my prior objections, I think having this change makes sense. |
@svrnm sounds good. Does that mean an approval? Don't want to merge this too quickly 🙂 |
Yes, approved :) Leaving it to you to merge it |
This is a bit of an aggressive way to address #1722
We may need to show how to use things like
context.with
in the instrumentation docs as there may be use cases that aren't adequately captured bystartActiveSpan
. But I think we can do that independently, and the main code samples involved are sorta just more complicated ways to do what's documented in the /instrumentation page.Also moves what is conceptual docs about span kind into the language-agnostic docs section for spans.