Skip to content

Implement Context Propagation through http headers#41

Merged
palazzem merged 5 commits into
masterfrom
bmermet/contextprop
Feb 19, 2018
Merged

Implement Context Propagation through http headers#41
palazzem merged 5 commits into
masterfrom
bmermet/contextprop

Conversation

@bmermet

@bmermet bmermet commented Feb 9, 2018

Copy link
Copy Markdown
Contributor

This implements a generic way of doing context propagation through the IHeaderCollection abstraction and implements it for HttpHeaders used by HttpClient and IHeaderDictionaryused by Asp.Net Core.

@bmermet bmermet requested a review from palazzem February 9, 2018 13:22
@bmermet bmermet added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Feb 9, 2018
@palazzem palazzem added this to the 0.1.4 milestone Feb 9, 2018
public void OnBeginRequest(HttpContext httpContext)
{
var scope = _tracer.StartActive("aspnet.request", serviceName: _serviceName);
var context = httpContext.Request.Headers.Extract();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this behavior should be under a specific setting (distributedTracing = true maybe?). It could happen that the service is a public exposed service without headers whitelist, and so clients could inject some arbitrary IDs to mess the trace.

Just to be clear: this is not a security issue, just a safe-guard to avoid controlling IDs from unwanted services.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍


namespace Datadog.Trace
{
// TODO test performance difference with a struct

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it's a long task, can we create something in our backlog? even if we're planning to solve all TODO before GA, having an idea of the numbers of TODO would be great. Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did the benchmark and it was worse. I removed the TODO.

/// </summary>
/// <param name="traceId">The trace identifier.</param>
/// <param name="spanId">The span identifier.</param>
public SpanContext(ulong traceId, ulong spanId)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've made this constructor public, meaning that anyone can extend the library to write their own context propagation.

@palazzem palazzem merged commit e7a74ba into master Feb 19, 2018
@palazzem palazzem deleted the bmermet/contextprop branch February 19, 2018 09:00
macrogreg added a commit that referenced this pull request Aug 20, 2021
* Address build warnings
* Address layout issues
* Address NuGet issues
* Clean-up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants