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

Documentation out of sync with library implementation for Span #1129

Open
nwhite-riot opened this issue Jan 27, 2025 · 5 comments · May be fixed by #1139
Open

Documentation out of sync with library implementation for Span #1129

nwhite-riot opened this issue Jan 27, 2025 · 5 comments · May be fixed by #1139

Comments

@nwhite-riot
Copy link

The Sentry documentation for Span indicates that parent_span_id is optional, but the native library requires that a valid parent Span object be passed in, otherwise the Span creation fails and returns NULL.

Even if the divergence here is due to library expectations (for example that Span is created as a child of a Transaction only), I think the library should acknowledge and explain the behavior difference.

@supervacuus
Copy link
Collaborator

I'm sorry, but you point to two places with no contract between them.

Your first link points to the payload documentation of the develop docs (i.e., SDK or endpoint implementer documentation). This is neither SDK documentation nor common user documentation for a particular feature. This is the contract when you want to talk to the relay endpoint directly, which makes the parent_span_id optional because transactions being spans won't have a parent_span_id.

The function you point to starts a "child" at whichever span you give it as a parent, from which it inherits properties. This function doesn't make any sense with a "null"-parent because that would mean it wouldn't be creating a child.

If you think you need an orphan/root span constructor, I can only refer you to the tracing docs, which document the behavior that we expose in our API which particularly states (emphasis mine):

Top-level spans can be broken down into smaller spans, mirroring the way one function may call others. Every span may be the parent span to multiple child spans. One span in every transaction represents the transaction itself, with all other spans descending from that root span.

Meaning there is no reason for SDK users to produce orphan/root-spans even though the contract to the server-side endpoint would theoretically allows this.

There is an ongoing initiative that targets the removal of transactions entirely from the SDKs, but it isn't the current standard across the SDKs. For this initiative it will make sense to expose SDK APIs that handle orphan spans.

Let me know if and where you think this makes the most sense to add as context.

@nwhite-riot
Copy link
Author

In principle I agree, but sentry.h pushes the user towards the documentation for information about the properties, so it may not be as obvious that these things are not bound by the same contract to folks who are less familiar with the library.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 27, 2025
@supervacuus
Copy link
Collaborator

In principle I agree, but sentry.h pushes the user towards the documentation for information about the properties, so it may not be as obvious that these things are not bound by the same contract to folks who are less familiar with the library.

That's fair. I will adapt the docs to clarify.

@JoshuaMoelans
Copy link
Member

Hi @nwhite-riot , thanks for raising this. I updated the docstrings for the span-creating functions. Is it clearer now why the span interface docs & the native code differ?

@nwhite-riot
Copy link
Author

Hi @nwhite-riot , thanks for raising this. I updated the docstrings for the span-creating functions. Is it clearer now why the span interface docs & the native code differ?

Yes this is definitely helpful! Thank you :)

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for: Product Owner
Status: In Progress
Development

Successfully merging a pull request may close this issue.

3 participants