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

Enhance existing http example to support w3c trace context propagation #727

Merged
merged 14 commits into from
May 8, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented May 6, 2021

Changes

Please provide a brief description of the changes here.

This PR enhances the existing HTTP client/serve example with http propagation. The trace_id and parent_span_id are propagated from client to server.
Also nit change on renaming the header file extension as per the convention (hpp -> h)

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team May 6, 2021 09:04
@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #727 (ddf413b) into main (4027edb) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #727   +/-   ##
=======================================
  Coverage   94.75%   94.75%           
=======================================
  Files         217      217           
  Lines        9923     9923           
=======================================
  Hits         9403     9403           
  Misses        520      520           
Impacted Files Coverage Δ
.../include/opentelemetry/common/key_value_iterable.h 100.00% <ø> (ø)

@lalitb lalitb changed the title Enhance existing http example with w3c trace context propagation Enhance existing http example to support w3c trace context propagation May 6, 2021
auto prop = opentelemetry::context::propagation::GlobalTextMapPropagator::GetGlobalPropagator();
auto current_ctx = opentelemetry::context::RuntimeContext::GetCurrent();
auto new_context = prop->Extract(carrier, current_ctx);
options.parent = GetSpanFromContext(new_context)->GetContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this shouldn't be strictly necessary, as Extract sets the span as the currently active span in the context?

Copy link
Member Author

@lalitb lalitb May 7, 2021

Choose a reason for hiding this comment

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

@pyohannes - Extract would return a new context with the span created with extracted span_context, and won't make it active. This is as per the specs :

Returns a new Context derived from the Context passed as argument.

#include <cstring>
#include <iostream>
#include <vector>
#include "opentelemetry/ext/http/client/http_client.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can eventually generalize some of this, since the same / similar logic would be necessary for gRPC metadata. I like how it was handled in dapr examples:

https://docs.dapr.io/developing-applications/building-blocks/observability/w3c-tracing/w3c-tracing-howto/

Copy link
Member Author

@lalitb lalitb May 7, 2021

Choose a reason for hiding this comment

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

Yes, we should eventually reuse part of the code for both HTTP and grpc.

Copy link
Contributor

@maxgolov maxgolov left a comment

Choose a reason for hiding this comment

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

I'm good with this for now. We may need to refactor this to generalize the common parts to be agnostic of whether it's HTTP client headers or gRPC metadata.

@lalitb lalitb merged commit d8467a7 into open-telemetry:main May 8, 2021
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.

3 participants