Added support for Trace name truncation for traces #3689

aantono · 2018-07-27T18:23:53Z

What does this PR do?

In many situations the trace name gets pretty large (over 100 chars, especially when using k8s provider). This provides an option to truncate the name to a configurable length to avoid dropped traces.

More

Added/updated tests
Added/updated documentation

timoreimann

As discussed on Slack, we decided to introduce an option that allows users to define the span name length threshold, with the default being that no truncation happens. We also need this to ensure that we don't break existing clients.

The outlined behavior isn't reflected in the PR yet, or is it?

aantono · 2018-07-30T14:16:13Z

@timoreimann You are right, changed the default to be no truncation.

mmatur

Thanks for your contribution @aantono.
First quick review

mmatur · 2018-07-30T15:58:15Z

middlewares/tracing/entrypoint.go

- opNameFunc := func(r *http.Request) string {
- return fmt.Sprintf("Entrypoint %s %s", e.entryPoint, r.Host)
- }
+ //opNameFunc := func(r *http.Request) string {


Could you remove these comments please

mmatur · 2018-07-30T16:54:00Z

middlewares/tracing/entrypoint_test.go

+package tracing
+
+import (
+ "github.com/opentracing/opentracing-go/ext"


Please sort your imports

mmatur · 2018-07-30T16:54:39Z

middlewares/tracing/entrypoint_test.go

+ r *http.Request
+ next http.HandlerFunc
+ }
+ tests := []struct {


Could you please rename tests into testCases

mmatur · 2018-07-30T16:55:29Z

middlewares/tracing/entrypoint_test.go

+ }
+ for _, tt := range tests {
+ t.Run(tt.desc, func(t *testing.T) {
+ defaultMockSpan.Reset()


Could you please run in parallel

Those are run serial on purpose, as the Mock tracer is static and otherwise would be having race conditions.

mmatur · 2018-07-30T16:55:48Z

middlewares/tracing/entrypoint_test.go

+ },
+ },
+ }
+ for _, tt := range tests {


Could you please rename tt in test

mmatur · 2018-07-30T17:04:53Z

middlewares/tracing/tracing_test.go

+ }
+ for _, tt := range tests {
+ t.Run(tt.desc, func(t *testing.T) {
+ if got := ComputeHash(tt.text); got != tt.want {


Could you please run your test in parallel

mmatur · 2018-07-30T17:05:10Z

middlewares/tracing/tracing_test.go

+ want: "f9b0078b",
+ },
+ }
+ for _, tt := range tests {


Could you please add a new line before

mmatur · 2018-07-30T17:05:15Z

middlewares/tracing/tracing_test.go

+ want: "some-service-100.slug.namespace.envi...",
+ },
+ }
+ for _, tt := range tests {


Could you please add a new line before

mmatur · 2018-07-30T17:05:36Z

middlewares/tracing/tracing_test.go

+ want: "some-service-100.slug.namespace.envi...",
+ },
+ }
+ for _, tt := range tests {


Could you please rename tt into test

mmatur · 2018-07-30T17:05:59Z

middlewares/tracing/tracing_test.go

+ }
+ for _, tt := range tests {
+ t.Run(tt.desc, func(t *testing.T) {
+ if got := TruncateString(tt.text, tt.limit); got != tt.want || len(got) > tt.limit {


Could you please run testCases in parallel

mmatur

Thanks for your fix @aantono
I created a PR aantono#1 on your fork to improve tests. Could you have a look please

mmatur

LGTM :)
Great PR @aantono :)

juliens

LGTM

dtomcej

HASH ALL THE THINGS
LGTM

aantono · 2018-07-31T16:06:55Z

Rebased against latest v1.7

timoreimann

Left some comments.

Shouldn't we need to update some documentation as well?

timoreimann · 2018-07-31T20:39:34Z

middlewares/tracing/entrypoint.go

+
+ if spanLimit > 0 && len(name) > spanLimit {
+ if spanLimit < EntryPointMaxLengthNumber {
+ log.Warnf("SpanNameLimit is set to be less then required static number of characters, defaulting to %d + 3", EntryPointMaxLengthNumber)


Grammar: then -> than

timoreimann · 2018-07-31T20:42:58Z

middlewares/tracing/tracing.go

@@ -13,13 +14,23 @@ import (
 "github.com/opentracing/opentracing-go/ext"
 )

+// ForwardMaxLengthNumber defines the number of static characters in the Forwarding Span Trace name - 8 chars for 'forward ' + 8 chars for hash + 2 chars for '_'


My brain mistook the - in Trace name - 8 for a minus shortly due to the other arithmetic operator (+) used here. Can we use a colon (:) instead?

timoreimann · 2018-07-31T20:43:04Z

middlewares/tracing/tracing.go

+// ForwardMaxLengthNumber defines the number of static characters in the Forwarding Span Trace name - 8 chars for 'forward ' + 8 chars for hash + 2 chars for '_'
+const ForwardMaxLengthNumber = 18
+
+// EntryPointMaxLengthNumber defines the number of static characters in the Entrypoint Span Trace name - 11 chars for 'Entrypoint ' + 8 chars for hash + 2 chars for '_'


Colon here too please.

timoreimann · 2018-07-31T20:45:17Z

middlewares/tracing/entrypoint.go

@@ -40,3 +38,19 @@ func (e *entryPointMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request,
 LogResponseCode(span, recorder.Status())
 span.Finish()
 }
+
+func generateEntryPointSpanName(r *http.Request, entryPoint string, spanLimit int) string {


Can we please add a GoDoc to the function explaining what it does, along with an example?

Please include why we do + 3 when we hit the spanLimit < EntryPointMaxLengthNumber case.

timoreimann · 2018-07-31T20:48:45Z

middlewares/tracing/entrypoint_test.go

+ expectedName: "Entrypoint te... ww... 39b97e58",
+ },
+ {
+ desc: "no truncation test",


I'd put this test case first: reading it first helps to understand the other cases.

timoreimann · 2018-07-31T21:04:42Z

middlewares/tracing/tracing.go

@@ -160,3 +171,28 @@ func SetErrorAndWarnLog(r *http.Request, format string, args ...interface{}) {
 log.Warnf(format, args...)
 LogEventf(r, format, args...)
 }
+
+// TruncateString reduces the length of the 'str' argument to 'num' - 3 and adds a '...' suffix to the tail


Trailing punctuation missing.

timoreimann · 2018-07-31T21:05:57Z

middlewares/tracing/tracing.go

@@ -160,3 +171,28 @@ func SetErrorAndWarnLog(r *http.Request, format string, args ...interface{}) {
 log.Warnf(format, args...)
 LogEventf(r, format, args...)
 }
+
+// TruncateString reduces the length of the 'str' argument to 'num' - 3 and adds a '...' suffix to the tail
+func TruncateString(str string, num int) string {


The function does not need to be exported, does it?

timoreimann · 2018-07-31T21:06:52Z

middlewares/tracing/tracing.go

+ return text
+}
+
+// ComputeHash returns the first TraceNameHashLength character of the sha256 hash for 'name' argument


Trailing punctuation missing.

timoreimann · 2018-07-31T21:07:57Z

middlewares/tracing/tracing.go

+ data := []byte(name)
+ hash := sha256.New()
+ _, err := hash.Write(data)
+ if err != nil {


Can be merged with previous line:

if err := has.Write(data); err != nil {

timoreimann · 2018-07-31T21:09:28Z

middlewares/tracing/tracing_test.go

+ expected: "some ve...",
+ },
+ {
+ desc: "short text less than limit 10",


Please put this test case first.

timoreimann · 2018-07-31T21:44:13Z

middlewares/tracing/entrypoint.go

@@ -40,3 +38,20 @@ func (e *entryPointMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request,
 LogResponseCode(span, recorder.Status())
 span.Finish()
 }
+
+// generateEntryPointSpanName will return a Span name of an appropriate lenth based on the 'spanLimit' argument. If needed, it will be truncated, but will not be less than 24 characters


Trailing punctuation missing.

timoreimann · 2018-07-31T21:44:26Z

middlewares/tracing/forwarder.go

+ }
+ hash := ComputeHash(name)
+ limit := (spanLimit - ForwardMaxLengthNumber) / 2
+ name = fmt.Sprintf("forward %s/%s/%s", TruncateString(frontend, limit), TruncateString(backend, limit), hash)


timoreimann · 2018-07-31T21:44:38Z

middlewares/tracing/tracing.go

 func SetErrorAndWarnLog(r *http.Request, format string, args ...interface{}) {
 SetError(r)
 log.Warnf(format, args...)
 LogEventf(r, format, args...)
 }
+
+// TruncateString reduces the length of the 'str' argument to 'num' - 3 and adds a '...' suffix to the tail.


timoreimann · 2018-07-31T21:45:10Z

middlewares/tracing/forwarder.go

+ }
+
+ return name
+}


Gotcha, fine with me. 👍

timoreimann · 2018-07-31T21:47:24Z

docs/configuration/tracing.md

@@ -22,6 +22,12 @@ Træfik supports two backends: Jaeger and Zipkin.
 # Default: "traefik"
 #
 serviceName = "traefik"
+
+ # Span name limit allows for name truncation in case of very long Frontend/Backend names


Can we append something like

"This can prevent certain tracing providers from dropping such traces."

?

Similar below.

timoreimann

LGTM. 👏

traefiker added area/middleware size/L status/0-needs-triage labels Jul 27, 2018

traefiker added this to the 1.7 milestone Jul 27, 2018

aantono force-pushed the dd-apm branch from 9d1600c to 4552ccb Compare July 27, 2018 18:49

dtomcej added status/2-needs-review area/middleware/tracing and removed status/0-needs-triage area/middleware labels Jul 27, 2018

aantono force-pushed the dd-apm branch from 4552ccb to aa1db10 Compare July 27, 2018 19:01

timoreimann suggested changes Jul 28, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 28, 2018

mmatur added kind/enhancement a new or improved feature. kind/bug/fix a bug fix and removed contributor/waiting-for-corrections kind/enhancement a new or improved feature. labels Jul 30, 2018

mmatur requested changes Jul 30, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 30, 2018

mmatur requested changes Jul 31, 2018

View reviewed changes

aantono force-pushed the dd-apm branch from 6da1cf9 to d99ebb7 Compare July 31, 2018 14:48

mmatur approved these changes Jul 31, 2018

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Jul 31, 2018

juliens approved these changes Jul 31, 2018

View reviewed changes

dtomcej approved these changes Jul 31, 2018

View reviewed changes

aantono force-pushed the dd-apm branch from d99ebb7 to 79b7729 Compare July 31, 2018 16:06

timoreimann suggested changes Jul 31, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 31, 2018

Added support for Trace name truncation for traces

14aedc7

aantono and others added 4 commits July 31, 2018 16:39

Changed default tracing setting to not do truncation

7a7f015

Style cleanup

4a5f476

fix: improve truncating tracing (#1)

9436e0a

More syntax clean up

ed7a12b

aantono force-pushed the dd-apm branch from 79b7729 to ed7a12b Compare July 31, 2018 21:39

Added docs

195dc72

timoreimann suggested changes Jul 31, 2018

View reviewed changes

aantono added 2 commits July 31, 2018 16:50

Finishing touches... ;)

7eac285

Unexported TruncateString and ComputeHash

38d3c7c

timoreimann approved these changes Jul 31, 2018

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Jul 31, 2018

timoreimann added status/3-needs-merge and removed status/2-needs-review labels Jul 31, 2018

traefiker merged commit 4d79c2a into traefik:v1.7 Jul 31, 2018

traefiker removed the status/3-needs-merge label Jul 31, 2018

Added support for Trace name truncation for traces #3689

Added support for Trace name truncation for traces #3689

Conversation

aantono commented Jul 27, 2018 • edited Loading

What does this PR do?

More

timoreimann left a comment

Choose a reason for hiding this comment

aantono commented Jul 30, 2018

mmatur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

dtomcej left a comment

Choose a reason for hiding this comment

aantono commented Jul 31, 2018

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

aantono commented Jul 27, 2018 •

edited

Loading