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

:telemetry.span/3 documentation seems inconsistent #112

Closed
LostKobrakai opened this issue Jun 13, 2022 · 4 comments
Closed

:telemetry.span/3 documentation seems inconsistent #112

LostKobrakai opened this issue Jun 13, 2022 · 4 comments

Comments

@LostKobrakai
Copy link
Contributor

When providing StartMetadata and StopMetadata, these values will be sent independently to start and stop events. […] In general, StopMetadata should only provide values that are additive to StartMetadata so that handlers, such as those used for metrics, can rely entirely on the stop event.

It's not really clear from this if StartMetadata values needs to be supplied as StopMetadata as well if they should be available for both events. The code seems to suggest they should, the docs seem to suggest to not do that.

@josevalim
Copy link
Contributor

We don't perform any merging, so indeed the docs are inaccurate. PRs welcome!

@vanvoljg
Copy link
Contributor

Hello! I'm still a tad confused about this, even after the linked PR got merged.

In the quoted documentation paragraph, the docs mention that a reporter should be able to rely entirely on the stop event. However, this doesn't seem possible if StartMetadata isn't merged (automatically) into StopMetadata, because there is possibly a large amount of metadata that is unavailable to the stop event by default, and must be manually merged into the stop metadata when a span wrapper is implemented.

This seems to me that it makes it very easy to implement telemetry in a way which makes reporters very arduous to write (with need for long-running state to track and match span-related events via their references). I'm unsure if this is the intent, or if the documentation is incorrect, or if I'm just misunderstanding things entirely.

@bryannaegele
Copy link
Contributor

I'm not sure why that isn't the behavior.

@josevalim
Copy link
Contributor

However, this doesn't seem possible if StartMetadata isn't merged (automatically) into StopMetadata,

we should document that it is extremely Recommended for callers to return the stop metadata as a superset of the start metadata.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants