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

Demonstrate OpenTelemetry + dependency injection #154

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

glencbz
Copy link
Contributor

@glencbz glencbz commented Nov 10, 2023

Hi Temporal folks! Here's a doc change for something I figured out that I wish was called out in the docs. Feel free to reword this as you wish.

What was changed

Add an example of configuring OpenTelemetry with dependency injection to src/Temporalio.Extensions.OpenTelemetry/README.md.

Why?

src/Temporalio.Extensions.OpenTelemetry/README.md doesn't contain an example of how to configure OpenTelemetry when using dependency injection. It's pretty easy to figure out how to configure the client by also reading the src/Temporalio.Extensions.Hosting/README.md, but if one uses a hosted worker, it typically has to be configured separately. This is a common pitfall (according to
https://temporalio.slack.com/archives/C012SHMPDDZ/p1699016782885319?thread_ts=1698974629.231319&cid=C012SHMPDDZ at least), so document this.

Checklist

  1. Closes

NA

  1. How was this tested:

NA

  1. Any docs updates needed?

NA

@CLAassistant
Copy link

CLAassistant commented Nov 10, 2023

CLA assistant check
All committers have signed the CLA.

@glencbz glencbz force-pushed the otel-di-example branch 2 times, most recently from 966bdac to 1c65778 Compare November 14, 2023 01:57
@glencbz glencbz changed the title Demonstate OpenTelemetry + dependency injection Demonstrate OpenTelemetry + dependency injection Nov 14, 2023
@glencbz glencbz force-pushed the otel-di-example branch 2 times, most recently from b0acfb3 to 1b09094 Compare November 15, 2023 07:39
@glencbz
Copy link
Contributor Author

glencbz commented Nov 15, 2023

Updated and removed some cruft from the project I copy-pasted from.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Looks great! Just a minor pedantic thing about breaking up the snippet, then will merge. Thanks!

src/Temporalio.Extensions.OpenTelemetry/README.md Outdated Show resolved Hide resolved
src/Temporalio.Extensions.OpenTelemetry/README.md doesn't contain an
example of how to configure OpenTelemetry when using dependency
injection. It's pretty easy to figure out how to configure the client by
also reading the src/Temporalio.Extensions.Hosting/README.md, _but_ if
one uses a hosted worker, it typically has to be configured separately.
This is a common pitfall (according to
https://temporalio.slack.com/archives/C012SHMPDDZ/p1699016782885319?thread_ts=1698974629.231319&cid=C012SHMPDDZ
at least).

Add an example to src/Temporalio.Extensions.OpenTelemetry/README.md to
reduce the guesswork from users.
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Thanks! Will merge after CI completes.

@cretz cretz merged commit 32411c7 into temporalio:main Nov 16, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants