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

Rework baggage page to be less trace-specific #4692

Merged
merged 11 commits into from
Jun 26, 2024
Merged

Rework baggage page to be less trace-specific #4692

merged 11 commits into from
Jun 26, 2024

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Jun 15, 2024

Should fix #3674 and #1878 (comment)

Tried to capture how Baggage isn't trace-specific while still honoring the reality that it's usually used the most in conjunction with tracing.

cc @cijothomas

@cartermp cartermp requested a review from a team June 15, 2024 17:32
@cartermp
Copy link
Contributor Author

/fix:format

@opentelemetrybot
Copy link
Collaborator

@austinlparker
Copy link
Member

Would it be worth calling out somehow that baggage is only intended for Observability data, and that it's not recommended to use it to side channel values for other purposes (control flow, etc)?

@cartermp
Copy link
Contributor Author

@austinlparker added

@cartermp
Copy link
Contributor Author

/fix:format

@opentelemetrybot
Copy link
Collaborator

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Thank you! Some remarks

content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
content/en/docs/concepts/signals/baggage.md Outdated Show resolved Hide resolved
cartermp and others added 2 commits June 17, 2024 06:53
Co-authored-by: Severin Neumann <[email protected]>
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>

For example, in .NET you might do this:
To add Baggage entries to attributes, you need to explicitly read that data and
add it. For example, in .NET you might do this:
Copy link
Member

Choose a reason for hiding this comment

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

could we switch to use any language here other than .NET? ("Activity", "Tag" are unknown terms outside .NET, so best to keep this example neutral)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it -- decided it's not worth putting code samples in here right now.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have any languages that mention the baggage API already? If so we might consider having some listing similar to the sampling page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I'm aware of, no.

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if we can add new page/section for each language to show how to use the Baggage API.

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Api#baggage-api is what we kept in code repo for .NET so far. Every language would be already having similar content. We can copy/move them all to the docs website.


```csharp
var accountId = Baggage.GetBaggage("AccountId");
Activity.Current?.SetTag("AccountId", accountId);
```

Because one of the most common use cases for Baggage is to add data to
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if the part "Because one of the most common use cases" is relevant here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded a bit. It is common, maybe not the most common

@cartermp
Copy link
Contributor Author

/fix:format

@opentelemetrybot
Copy link
Collaborator

@cartermp
Copy link
Contributor Author

@theletterf any final thoughts?

Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

Approved!

@cartermp cartermp merged commit 2675bb4 into main Jun 26, 2024
18 checks passed
@cartermp cartermp deleted the cartermp-patch-1 branch June 26, 2024 19:23
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.

Baggage doc should be modified to make it less tied to tracing singnal
8 participants