Add WithoutSubSpans option to NewClientTrace #879

coryb · 2021-07-11T20:44:32Z

This adds an optional behavior to NewClientTrace so that it only
adds events and annotations to the span found in SpanFromContext.
The default behavior remains unchanged, several sub-spans will be
created for each request. This optional behavior is useful when
tracing "complex" processes that have many http calls. In this case
the added sub-spans become "noise" and may distract from the overall
trace.

This also adds several useful attributes from data available
in the httptrace callbacks:

http.conn.reused - bool if connection was reused
http.conn.wasidle - bool if connection was idle
http.conn.idletime - if wasidle, duration of idletime
http.conn.start.network - start connection network type [tcp/udp]
http.conn.done.network - end connection network type [tcp/udp]
http.conn.done.addr - connection address when done
http.dns.addrs - list of addrs returned from dns lookup

Fixes #876

tonistiigi · 2021-07-12T07:27:00Z

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go

+	value := sliceToString(v)
+	if _, ok := ct.redactedHeaders[k]; ok {
+		// redact all non-space runes to 'x'
+		value = strings.Map(func(r rune) rune {


I'd prefer if this wouldn't leak the length/spaces or waste space with long tokens. Just constant *** sgtm

I have even a stronger point of view. I think in such a case, the attribute should NOT be included at all. This is to ensure we do not leak any security-related internals.

Reasoning: open-telemetry/opentelemetry-specification#1502 (comment)

This would be also more in line with other specification recommendations such us:

https://github.com/open-telemetry/opentelemetry-specification/blame/main/specification/trace/semantic_conventions/http.md#L71

https://github.com/open-telemetry/opentelemetry-specification/blame/main/specification/trace/semantic_conventions/database.md#L160

For now I have updated to use fixed **** value. We can instead drop those annotations completely, I dont really have a preference. If we drop them, I guess I would change the WithRedactedHeaders to WithIgnoredHeaders to alllow appending to the fixed list. We can also swap the default to not collect any headers unless a WithHeaders option is specified. For the edge case of trying to debug authentication-related issues, we could add WithRedactgnoredHeaders (or some such name) to keep headers in the spans but always hide the values.

I prefer the redacted value instead of ignoring one. Seeing which requests were authenticated and which were not is an important common feature in tracing HTTP traffic and can point to issues.

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go

codecov · 2021-07-13T15:36:56Z

Codecov Report

Merging #879 (f89b79d) into main (061e903) will increase coverage by 0.1%.
The diff coverage is 77.4%.

@@           Coverage Diff           @@
##            main    #879     +/-   ##
=======================================
+ Coverage   69.2%   69.4%   +0.1%     
=======================================
  Files         77      77             
  Lines       4850    4929     +79     
=======================================
+ Hits        3359    3423     +64     
- Misses      1353    1367     +14     
- Partials     138     139      +1

Impacted Files	Coverage Δ
...on/net/http/httptrace/otelhttptrace/clienttrace.go	`77.6% <77.4%> (+1.7%)`	⬆️

pellared · 2021-07-14T12:26:53Z

The default behavior remains unchanged,

I think this is not true b/c of the default redactedHeaders.

Also is it possible to remove the default redactedHeaders if someone really wants to trace them (e.g. for some local testing)? I think that security should be "enabled" by default but there should always be an option to explicitly configure to some insecure settings.

coryb · 2021-07-14T14:38:40Z

The default behavior remains unchanged,

I think this is not true b/c of the default redactedHeaders.

True, there is a behaviour change due to the redacted headers. I didn't know about the leaking headers when I opened the issue and made that comment.

Also is it possible to remove the default redactedHeaders if someone really wants to trace them (e.g. for some local testing)? I think that security should be "enabled" by default but there should always be an option to explicitly configure to some insecure settings.

We could add a WithInsecureHeaders to disable the redaction and reset the current list. Motivated users could always just add an http.RoundTrip wrapper to their client that adds more attributes to the spans. Either option seems fine to me, curious if others have opinions...

coryb · 2021-07-17T21:06:58Z

curious if others have opinions...

No response, so I went ahead and added WithInsecureHeaders to facilitate the "debugging auth issues" use-case. Also extended the tests to verify WithoutHeaders, WithRedactedHeaders, and the new WithInsecureHeaders so there should not be a drop in test coverage.

CHANGELOG.md

coryb · 2021-07-20T14:45:40Z

Updated lint issue that broke the tests, I forgot to run the precommit check but it is working now.

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go

CHANGELOG.md

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go

instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go

CHANGELOG.md

thaJeztah · 2021-08-19T18:27:55Z

@coryb needs a rebase

coryb · 2021-08-20T01:40:39Z

Thanks for the ping, rebased, again. Hopefully we can get this merged in, please.

coryb · 2021-08-20T19:07:45Z

Could someone please poke the workflow approval button so the test will run?

coryb · 2021-08-23T16:34:10Z

Can we please get this merged in? We have two approvals and tests are passing ... what more is left?

thaJeztah · 2021-08-27T16:07:06Z

@coryb needs a rebase (again 😞)

This adds an optional behavior to NewClientTrace so that it only adds events and annotations to the span found in SpanFromContext. The default behavior remains unchanged, several sub-spans will be created for each request. This optional behavior is useful when tracing "complex" processes that have many http calls. In this case the added sub-spans become "noise" and may distract from the overall trace. This also adds several useful attributes from data available in the httptrace callbacks: http.conn.reused - bool if connection was reused http.conn.wasidle - bool if connection was idle http.conn.idletime - if wasidle, duration of idletime http.conn.start.network - start connection network type [tcp/udp] http.conn.done.network - end connection network type [tcp/udp] http.conn.done.addr - connection address when done http.dns.addrs - list of addrs returned from dns lookup Signed-off-by: coryb <cbennett@netflix.com>

On testing we learned that sensitive information was being stored in the traces. To prevent the security leak several security or sensitive headers will now be redacted. These are the headers redacted by default: Authorization WWW-Authenticate Proxy-Authenticate Proxy-Authorization Cookie Set-Cookie Users can add more headers to redact with WithRedactedHeaders. To disable adding headers to the span entirely users can use WithoutHeaders. Signed-off-by: coryb <cbennett@netflix.com> Co-authored-by: Robert Pająk <pellared@hotmail.com>

coryb · 2021-08-30T15:13:17Z

Hi folks, I have rebased again to address merge conflicts on the CHANGELOG. It has now been a month since the last change other than the rebases, so this is pretty frustrating.

I need someone to poke the workflow approval button again.

thaJeztah · 2021-09-02T12:20:57Z

@pellared @Aneurysm9 are you able to help out to this PR moving? 🤗

thaJeztah · 2021-09-02T16:11:09Z

Thanks!

coryb requested review from Aneurysm9, dashpole, evantorrie, jmacd, MadVikingGod, MrAlias, paivagustavo, pellared and XSAM as code owners July 11, 2021 20:44

coryb force-pushed the httptrace-without-subspans branch from 7ac081e to ce76e05 Compare July 11, 2021 20:45

tonistiigi mentioned this pull request Jul 12, 2021

tracing: update to otelhttp roundtripper moby/buildkit#2238

Merged

tonistiigi reviewed Jul 12, 2021

View reviewed changes

coryb force-pushed the httptrace-without-subspans branch from 5801e3b to 70ab82b Compare July 12, 2021 15:11

Aneurysm9 reviewed Jul 12, 2021

View reviewed changes

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go Outdated Show resolved Hide resolved

coryb force-pushed the httptrace-without-subspans branch 2 times, most recently from 2ceef5c to d757e2b Compare July 13, 2021 15:22

coryb force-pushed the httptrace-without-subspans branch from d757e2b to cc0c21d Compare July 17, 2021 21:04

pellared reviewed Jul 19, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

coryb force-pushed the httptrace-without-subspans branch 3 times, most recently from a32efea to 2188076 Compare July 20, 2021 14:43

Aneurysm9 approved these changes Jul 23, 2021

View reviewed changes

pellared reviewed Jul 26, 2021

View reviewed changes

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go Outdated Show resolved Hide resolved

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go Show resolved Hide resolved

coryb force-pushed the httptrace-without-subspans branch from 2188076 to ea594c2 Compare July 27, 2021 20:00

pellared reviewed Jul 28, 2021

View reviewed changes

pellared approved these changes Aug 11, 2021

View reviewed changes

Aneurysm9 reviewed Aug 11, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

coryb force-pushed the httptrace-without-subspans branch from f87cd46 to a0dc0a0 Compare August 11, 2021 23:02

coryb force-pushed the httptrace-without-subspans branch from a0dc0a0 to 47dbbcc Compare August 20, 2021 01:37

coryb force-pushed the httptrace-without-subspans branch 2 times, most recently from bbf1e35 to b8da8cd Compare August 20, 2021 01:44

coryb force-pushed the httptrace-without-subspans branch from b8da8cd to 81f719d Compare August 30, 2021 14:58

coryb and others added 2 commits August 30, 2021 07:59

coryb force-pushed the httptrace-without-subspans branch from 81f719d to 2756490 Compare August 30, 2021 15:02

Merge branch 'main' into httptrace-without-subspans

f89b79d

Aneurysm9 merged commit 265ebea into open-telemetry:main Sep 2, 2021

thaJeztah mentioned this pull request Sep 2, 2021

otelhttp: get tracer from current context if not set in constructor #873

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WithoutSubSpans option to NewClientTrace #879

Add WithoutSubSpans option to NewClientTrace #879

coryb commented Jul 11, 2021

tonistiigi Jul 12, 2021

pellared Jul 12, 2021

coryb Jul 12, 2021

tonistiigi Jul 12, 2021

codecov bot commented Jul 13, 2021 •

edited

pellared commented Jul 14, 2021

coryb commented Jul 14, 2021

coryb commented Jul 17, 2021

coryb commented Jul 20, 2021

thaJeztah commented Aug 19, 2021

coryb commented Aug 20, 2021

coryb commented Aug 20, 2021

coryb commented Aug 23, 2021

thaJeztah commented Aug 27, 2021

coryb commented Aug 30, 2021

thaJeztah commented Sep 2, 2021

thaJeztah commented Sep 2, 2021

Add WithoutSubSpans option to NewClientTrace #879

Add WithoutSubSpans option to NewClientTrace #879

Conversation

coryb commented Jul 11, 2021

tonistiigi Jul 12, 2021

Choose a reason for hiding this comment

pellared Jul 12, 2021

Choose a reason for hiding this comment

coryb Jul 12, 2021

Choose a reason for hiding this comment

tonistiigi Jul 12, 2021

Choose a reason for hiding this comment

codecov bot commented Jul 13, 2021 • edited

Codecov Report

pellared commented Jul 14, 2021

coryb commented Jul 14, 2021

coryb commented Jul 17, 2021

coryb commented Jul 20, 2021

thaJeztah commented Aug 19, 2021

coryb commented Aug 20, 2021

coryb commented Aug 20, 2021

coryb commented Aug 23, 2021

thaJeztah commented Aug 27, 2021

coryb commented Aug 30, 2021

thaJeztah commented Sep 2, 2021

thaJeztah commented Sep 2, 2021

codecov bot commented Jul 13, 2021 •

edited