Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(routingprocessor): add option to drop resource attribute used for routing #8990

Conversation

pmalek-sumo
Copy link
Contributor

Description: add an option to drop the attribute used for routing

Link to tracking Issue: #7407

Testing: Added UTs

Documentation: Added change in processor's README.

@pmalek-sumo pmalek-sumo force-pushed the routingprocessor-add-option-to-drop-routing-attribute branch from 19372c0 to 34f0e08 Compare March 31, 2022 18:55
@jpkrohling
Copy link
Member

I'll take a look soon, but I might have the chance to look at it only next week (Tue perhaps).

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments, but LGTM overall.

@@ -22,6 +22,7 @@ The following settings can be optionally configured:
- `attribute_source` defines where to look for the attribute in `from_attribute`. The allowed values are:
- `context` (the default) - to search the [context][context_docs], which includes HTTP headers
- `resource` - to search the resource attributes.
- `drop_resource_routing_attribute` - controls whether to remove the resource attribute used for routing. This is only relevant if AttributeSource is set to resource.
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit verbose for my taste, but I can't come up with a better name :-)

processor/routingprocessor/config.go Outdated Show resolved Hide resolved
processor/routingprocessor/processor_test.go Outdated Show resolved Hide resolved
processor/routingprocessor/processor_test.go Outdated Show resolved Hide resolved
processor/routingprocessor/processor_test.go Outdated Show resolved Hide resolved
processor/routingprocessor/processor_test.go Outdated Show resolved Hide resolved
assert.False(t, ok, "routing attribute should be dropped")
v, ok := attrs.Get("attr")
assert.True(t, ok, "non routing attributes shouldn't be dropped")
assert.Equal(t, "acme", v.StringVal())
Copy link
Member

Choose a reason for hiding this comment

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

ditto for those two lines

processor/routingprocessor/processor_test.go Outdated Show resolved Hide resolved
assert.False(t, ok, "routing attribute should be dropped")
v, ok := attrs.Get("attr")
assert.True(t, ok, "non routing attributes shouldn't be dropped")
assert.Equal(t, "acme", v.StringVal())
Copy link
Member

Choose a reason for hiding this comment

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

Same as the other comments

processor/routingprocessor/processor_test.go Show resolved Hide resolved
Collector automation moved this from In progress to Reviewer approved Apr 8, 2022
Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
@pmalek-sumo pmalek-sumo force-pushed the routingprocessor-add-option-to-drop-routing-attribute branch from 6f68233 to 4f3acce Compare April 8, 2022 14:55
@pmalek-sumo
Copy link
Contributor Author

Unrelated test failure:

2022-04-08T18:09:50.3893534Z make -C ./processor/tailsamplingprocessor test
2022-04-08T18:09:50.3912312Z make[2]: Entering directory '/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/processor/tailsamplingprocessor'
2022-04-08T18:09:50.3965173Z go test -race -v -timeout 300s --tags="" ./...
2022-04-08T18:09:57.2862836Z === RUN   TestCompositeHelper
2022-04-08T18:09:57.2863428Z --- PASS: TestCompositeHelper (0.00s)
2022-04-08T18:09:57.2863715Z === RUN   TestLoadConfig
2022-04-08T18:09:57.2864941Z --- PASS: TestLoadConfig (0.02s)
2022-04-08T18:09:57.2872557Z === RUN   TestCreateDefaultConfig
2022-04-08T18:09:57.2872991Z --- PASS: TestCreateDefaultConfig (0.00s)
2022-04-08T18:09:57.2873298Z === RUN   TestCreateProcessor
2022-04-08T18:09:57.2873635Z --- PASS: TestCreateProcessor (0.02s)
2022-04-08T18:09:57.2873938Z === RUN   TestSequentialTraceArrival
2022-04-08T18:09:57.2874321Z --- PASS: TestSequentialTraceArrival (0.16s)
2022-04-08T18:09:57.2874638Z === RUN   TestConcurrentTraceArrival
2022-04-08T18:09:57.2875007Z --- PASS: TestConcurrentTraceArrival (1.26s)
2022-04-08T18:09:57.2875349Z === RUN   TestSequentialTraceMapSize
2022-04-08T18:09:57.2875722Z --- PASS: TestSequentialTraceMapSize (0.61s)
2022-04-08T18:09:57.2876020Z === RUN   TestConcurrentTraceMapSize
2022-04-08T18:09:57.2876368Z race: limit on 8128 simultaneously alive goroutines is exceeded, dying
2022-04-08T18:09:57.2876957Z FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor	5.792s

@jpkrohling jpkrohling merged commit 8bbf27c into open-telemetry:main Apr 12, 2022
@sumo-drosiek sumo-drosiek deleted the routingprocessor-add-option-to-drop-routing-attribute branch April 13, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

None yet

4 participants