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

x-cloud-trace-context pattern was changed by google when setup gRPC http2.0 on Cloud RUN #167

Closed
paralelocslucasmagalhaes opened this issue Oct 21, 2021 · 5 comments · Fixed by #181
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@paralelocslucasmagalhaes

Hi guys.. I tested the Cloud Trace Propagator on Cloud Run running a gRPC service with HTTP 2.0 enabled and figure out a problem.

On my tests I notice the pattern of x-cloud-trace-context the Google's Services send was different than "trace_id/span_id;o=Flag". On my test I've received just "trace_id/span_id" without the ";o=Flag".

So.. the actual code implemented at Cloud Trace Propagator Class does "match = re.fullmatch(_TRACE_CONTEXT_HEADER_RE, header)" The re expression wasn't fullfiled and return None.

I have a suspicion that the change could be part of HTTP2.0 setup, cause others "REST Service" running on HTTP1.0 the x-cloud-trace-context pattern is the way we expected.

Originally posted by @paralelocslucasmagalhaes in #109 (comment)

@aabmass
Copy link
Collaborator

aabmass commented Oct 27, 2021

On my tests I notice the pattern of x-cloud-trace-context the Google's Services send

Which google services do you mean here? A Google service is calling your gRPC service?

On my tests I notice the pattern of x-cloud-trace-context the Google's Services send was different than "trace_id/span_id;o=Flag". On my test I've received just "trace_id/span_id" without the ";o=Flag".

Regardless of the cause, I think you're right that the ;o=Flag part should be optional–that's what we have in the JS propagator at least: https://github.com/GoogleCloudPlatform/opentelemetry-operations-js/blob/ddbfee05c77e36b24fdf308325429128b801b216/packages/opentelemetry-cloud-trace-propagator/src/CloudPropagator.ts#L74

@aabmass aabmass added good first issue Good for newcomers help wanted Extra attention is needed labels Oct 27, 2021
@punya
Copy link
Contributor

punya commented Jan 10, 2022

@paralelocslucasmagalhaes, is this problem still relevant to you?

@kamalmarhubi
Copy link

It seems clear that there should be agreement across Google's libraries. I think the single character code change + related tests is the correct thing to do here?

@kamalmarhubi
Copy link

kamalmarhubi commented Apr 11, 2022

@punya @aabmass could you review and merge #181 then make a release? The propagator is basically broken without that change. This lost a team member at least several hours investigating why transforming load balancer logs into otel spans wasn't working. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
4 participants