Skip to content
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

Update OTel Custom Instrumentation Pages #22967

Merged
merged 21 commits into from
May 9, 2024

Conversation

brett0000FF
Copy link
Contributor

@brett0000FF brett0000FF commented May 1, 2024

What does this PR do? What is the motivation?

  • Improve docs for OTel custom instrumentation.
  • Add more use case examples.
  • Add more context around required imports.
  • Add new shortcode for consistent and streamlined intro to each page (layouts/shortcodes/otel-custom-instrumentation-lang.md).

Teams to review:

  • Java
  • Python
  • Node
  • .NET
  • Go
  • PHP

Merge instructions

  • Please merge after reviewing

Additional notes

@brett0000FF brett0000FF requested review from a team as code owners May 1, 2024 22:55
@github-actions github-actions bot added the Architecture Everything related to the Doc backend label May 1, 2024
@brett0000FF brett0000FF added the WORK IN PROGRESS No review needed, it's a wip ;) label May 1, 2024
@brett0000FF brett0000FF removed the WORK IN PROGRESS No review needed, it's a wip ;) label May 2, 2024
Copy link
Contributor

@estherk15 estherk15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, left some suggestions!

@brett0000FF
Copy link
Contributor Author

Looks good, left some suggestions!

@estherk15 - Awesome, thank you for the great suggestions!

Copy link
Contributor

@PerfectSlayer PerfectSlayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @brett0000FF 👋

Sorry the the change request 😞
They shared me your PR for a quick review as the developer of OTel support the Java tracer, but I think you might need to pair with @bm1549 team to help you fix the code samples: some are to avoid completely, some are not doing what they are for, one might not compile.

It's nothing bad or hard to fix, but just make sure to get them tested (compile and run as expected) before publishing them 🙏 I am part of platform team now but you should be in good hands with @bm1549


- Datadog Java tracing library `dd-trace-java` version 1.10.0 or greater.
<div class="alert alert-info">OpenTelemetry is supported in Java after version 1.24.0.</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes the decision of dropping 1.10?
1.24 introduces the new span naming and span links only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tushar-datadog - I think you added this change to our draft. Do you have an answer for this? Thanks!

});
}
{{< /highlight >}}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This page has all the code examples for how to start a span:

https://datadoghq.dev/dd-trace-js/interfaces/export_.opentelemetry.Tracer.html

One important thing to note and mention is that the nodejs tracer doesn't currently support the Otel Context API, so user's should be advised against creating spans from otel contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look! We originally had a table describing OTel features that are not supported by each tracer, but that was leading to confusion for those that are unfamiliar with OTel. I'll take a look and see where we could add this info about the Context API. 👍

@brett0000FF brett0000FF requested a review from link04 May 6, 2024 17:03
@brett0000FF brett0000FF requested a review from PerfectSlayer May 6, 2024 21:26
Copy link
Contributor

@mabdinur mabdinur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few non blocking comments. Overall this change looks good

Copy link
Contributor

@link04 link04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zacharycmontoya
Copy link
Contributor

Regarding PHP, the PHP developers I would loop in are not around until Friday. Though, based on our internal docs, the content here looks accurate to me. Let me know how you want to proceed.

@brett0000FF brett0000FF merged commit 012eccd into master May 9, 2024
12 checks passed
@brett0000FF brett0000FF deleted the brett0000FF/otel-api-updates branch May 9, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture Everything related to the Doc backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants