poc(ddtrace/tracer): migrate tracer config to knobs #3030
Conversation
darccio
changed the title
darccio
force-pushed
the
rdw/knobs-poc
branch
from
8666f78 to
589085a
Compare
Datadog ReportBranch report: ✅ 0 Failed, 5110 Passed, 67 Skipped, 2m 26.53s Total Time |
darccio
force-pushed
the
rdw/knobs-poc
branch
from
589085a to
6b7054d
Compare
BenchmarksBenchmark execution time: 2024-12-13 14:40:31 Comparing candidate commit 6bacb78 in PR branch Found 0 performance improvements and 9 performance regressions! Performance is the same for 49 metrics, 1 unstable metrics. scenario:BenchmarkPartialFlushing/Disabled-24
scenario:BenchmarkPartialFlushing/Enabled-24
scenario:BenchmarkSetTagStringer-24
scenario:BenchmarkSingleSpanRetention/no-rules-24
scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24
scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24
scenario:BenchmarkStartSpan-24
scenario:BenchmarkStartSpanConcurrent-24
scenario:BenchmarkTracerAddSpans-24
|
darccio
changed the title
darccio
force-pushed
the
rdw/knobs-poc
branch
2 times, most recently
from
7f88f14 to
f03ee9d
Compare
ddtrace/tracer/option.go
Outdated
| Parse: func(s string) (bool, error) { | ||
| if s == "" { | ||
| return true, nil | ||
| } | ||
| v, err := strconv.ParseBool(s) | ||
| if err != nil { | ||
| return true, err | ||
| } | ||
| return v, nil | ||
| }, |
There was a problem hiding this comment.
Why not use knobs.ToBool here?
There was a problem hiding this comment.
Because strconv.ParseBool("") returns an error, and it felt like a hidden behaviour.
ddtrace/tracer/option.go
Outdated
| Parse: func(v string) (int, error) { | ||
| i, _ := strconv.Atoi(v) | ||
| if i <= 0 { | ||
| log.Warn("DD_TRACE_PARTIAL_FLUSH_MIN_SPANS=%d is not a valid value, setting to default %d", i, 1000) |
There was a problem hiding this comment.
A thought about knobs that's been made clear by this example: the fact that the error log is so specific to environment variables makes me wonder again whether Parse should really be on EnvVar instead of Definition...
There was a problem hiding this comment.
I see what you mean. We can leave Parse for general use - for any possible string source - and recommend using Transform for this kind of logic.
There was a problem hiding this comment.
First issue: all the logic in that Parse function is coupled with the float parsing. So, the only issue is that logging messages 😁 This reinforces the idea of having some kind of validation function, and also delegating logging to Parse's caller.
darccio
force-pushed
the
rdw/knobs-poc
branch
from
f03ee9d to
e7f6893
Compare
| Transform: mapSampleRate, | ||
| }, | ||
| }, | ||
| Resolve: func(environ map[string]string, decision string) (string, error) { |
There was a problem hiding this comment.
@mtoffl01 As you already saw, I took the liberty to create a Resolve function implementing what we discussed offline. This Resolve allows to do extra validations in the whole context, but also potentially overriding what environment variable should be used.
BTW, I'm open to better naming 😆
What does this PR do?
Proof of concept using
github.com/darccio/knobsproposal for describing configuration.This PR depends on changes available in this PR's branch: darccio/knobs#5
Motivation
R&D week!
Reviewer's Checklist
v2-devbranch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!