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.

Sign up for GitHub

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

Jump to bottom

Instrument SpanContextFromContext to return the span context of the auto generated span #1033

Closed
wants to merge 1 commit into from
Closed

Instrument SpanContextFromContext to return the span context of the auto generated span #1033

wants to merge 1 commit into from

Conversation

RonFed
Copy link
Contributor

@RonFed RonFed commented Aug 25, 2024 •
edited by MrAlias
Loading

Instrument trace.SpanContextFromContext which allows returning the span context of auto-generated span if one exists.
Update e2e tests of net/http and go.opentelemetry.io/otel/internal/global to call trace.SpanContextFromContext and adding a check that the returned span context is the same as the one of the exported span.

Related: #987
Related: #974

@RonFed RonFed linked an issue Aug 25, 2024 that may be closed by this pull request
add support for trace.SpanContextFromContext so it is possible to get a tracer and span info to injected in logs #987
Open
@RonFed
Copy link
Contributor Author

RonFed commented Aug 25, 2024

Open questions:

  • Does this need to be under an opt-in flag? If so should it be the same flag for the global instrumentation? I am not sure since this is relevant for both "manual span which becomes auto" and for fully auto-generated spans like in the net/http test.
  • Both tests are calling a helper function which calls trace.SpanContextFromContext and outputs the result in a format that will be easy for us to verify in bats. Should we create a module for common e2e stuff (I think this will require an extra PR to use this module in the tests).
  • The probe in this PR is different from the current probes we have today because it does not generate events. This means that currently, the eBPF code must declare some dummy perf buffer that is not used, and the probe.Run function is not reading anything. Should we modify our probe interface or create a different implementation of it which will return from Run immediately and won't allocate a perf buffer in Load? Another option is to decompose the current interface to a base and then embed this interface in the current one.

RonFed
RonFed commented Aug 26, 2024
View reviewed changes
internal/pkg/instrumentation/bpf/net/http/server/bpf/probe.bpf.c
@@ -230,6 +230,7 @@ int uprobe_serverHandler_ServeHTTP(struct pt_regs *ctx)
start_span(&start_span_params);

bpf_map_update_elem(&http_server_uprobes, &key, uprobe_data, 0);
bpf_printk("server go_context.data: 0x%llx", go_context.data);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove

internal/test/e2e/nethttp/go.mod

require go.opentelemetry.io/otel v1.29.0 // indirect

replace go.opentelemetry.io/auto/internal/test => ../..
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove

@MrAlias
Copy link
Contributor

MrAlias commented Sep 17, 2024

Closing for now. The alternate SDK approach should make this unneeded. Will reopen if that changes.

@MrAlias MrAlias closed this Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@RonFed @MrAlias