pkg/trace/event: make event service & operation matching insensitive

[gbbr]

This change makes apm_config.analyzed_rate_by_service and apm_config.analyzed_spans
entries case insensitive.

The reasoning behind this is that Viper always lower-cases keys in the yaml file,
making it impossible to make case-sensitive comparisons against the orginal value.

See spf13/viper#411.

[bmermet]

Maybe we should enforce it ourselves on top of Viper? I don't like the fact that we depend so much on a side effect of a dependency. If Viper changes its behavior, extracting events for all uppercase names will break but none of our tests will catch it (at least as far as I can tell).

[gbbr]

I agree with your concern. How about if I were to add a specific test for it? Reading from a yaml with caps asserting that it gets lowercased? Also note that we have vendored Viper to the repository DataDog/Viper so it’s technically under our hands and with the added test we should be good to go. WDYT?

…

On Mon, 4 Mar 2019 at 15:05, Bertrand Mermet ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/trace/config/config.go <#3113 (comment)> : > @@ -101,7 +101,7 @@ type AgentConfig struct { // It maps tag keys to a set of replacements. Only supported in A6. ReplaceTags []*ReplaceRule - // transaction analytics + // transaction analytics, all map keys are lower-cased by Viper. Maybe we should enforce it ourselves on top of Viper? I don't like the fact that we depend so much on a side effect of a dependency. If Viper changes its behavior, extracting events for all uppercase names will break but none of our tests will catch it (at least as far as I can tell). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3113 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGYGlDr8agdx_g2ko2yWNE4QtI3jPXoDks5vTRougaJpZM4bcQ6D> .

[codecov]

Codecov Report

Merging #3113 into master will decrease coverage by 1.79%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #3113     +/-   ##
=========================================
- Coverage   56.03%   54.23%   -1.8%     
=========================================
  Files         407      540    +133     
  Lines       30189    38383   +8194     
=========================================
+ Hits        16917    20818   +3901     
- Misses      12424    16324   +3900     
- Partials      848     1241    +393

Flag	Coverage Δ
#linux	57.25% <100%> (?)
#windows	56.06% <100%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
pkg/trace/event/extractor_fixed_rate.go	100% <100%> (ø)	⬆️
pkg/trace/event/extractor_legacy.go	62.5% <100%> (+62.5%)	⬆️
...g/util/kubernetes/apiserver/metadata_controller.go	55.3% <0%> (ø)
pkg/util/kubernetes/apiserver/diagnosis.go	18.18% <0%> (ø)
pkg/util/containers/cri/diagnosis.go	28.57% <0%> (ø)
pkg/clusteragent/custommetrics/store.go	65.04% <0%> (ø)
pkg/util/hostname_nix.go	0% <0%> (ø)
pkg/util/docker/global.go	0% <0%> (ø)
pkg/util/pipe_nix.go	0% <0%> (ø)
pkg/util/kubernetes/apiserver/apiserver.go	11.91% <0%> (ø)
... and 146 more

[bmermet]

I agree that having an E2E test from config to the extractor is the best safeguard. Otherwise simply applying ToLower to the string we get from Viper to make sure we enforce it explicitly seems safe enough for me.

[gbbr]

I chose your latter suggestion and lower-cased all the keys on constructing these extractors. I guess from an API standpoint this is the best approach anyway. Let me know what you think.

[bmermet]

LGTM 👍