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

Add invert match on tail sampling string attribute #4393

Conversation

fabiovn
Copy link
Contributor

@fabiovn fabiovn commented Jul 30, 2021

Description: Adding option invert_match to string_attribute policy on tail sampling processor.

Motivation: We need to discard traces of health check and metrics endpoints. We tried to use negative lookahed but it is not supported by golang. Code was designed on a "whitelist" perspective so, we had to implement an alternative flow that supports "blacklist" perspective.

Testing: Existing tests were duplicated and this new option added to them.

Documentation: A new example policy was added to processor/tailsamplingprocessor/README.md that shows how to use it.

@fabiovn fabiovn requested review from jpkrohling and a team as code owners July 30, 2021 13:11
@fabiovn fabiovn force-pushed the tailsampling-stringattribute-invertmatch branch 2 times, most recently from f83a215 to 899589d Compare August 6, 2021 12:58
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@jpkrohling
Copy link
Member

Sorry for the delay, I'll review this today.

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.

LGTM. I think the test table could be simplified, but it's nothing serious and wouldn't block the merging.

{
Desc: "invert nonmatching node attribute key",
Trace: newTraceStringAttrs(map[string]pdata.AttributeValue{"non_matching": pdata.NewAttributeValueString("value")}, "", ""),
filterCfg: &TestStringAttributeCfg{Key: "example", Values: []string{"value"}, EnabledRegexMatching: false, CacheMaxSize: defaultCacheSize, InvertMatch: true},
Copy link
Member

Choose a reason for hiding this comment

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

Is the regex setting here decisive for the test? If not, leave it out. Same for the cache max size, and same for the other assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regex setting is necessary on this case. I duplicated all tests to ensure backward compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, looks good then.

@bogdandrutu
Copy link
Member

Please rebase so we ensure tests pass at head :)

@fabiovn fabiovn force-pushed the tailsampling-stringattribute-invertmatch branch from 899589d to caa761c Compare August 19, 2021 22:03
{
Desc: "invert nonmatching node attribute key",
Trace: newTraceStringAttrs(map[string]pdata.AttributeValue{"non_matching": pdata.NewAttributeValueString("value")}, "", ""),
filterCfg: &TestStringAttributeCfg{Key: "example", Values: []string{"value"}, EnabledRegexMatching: false, CacheMaxSize: defaultCacheSize, InvertMatch: true},
Copy link
Member

Choose a reason for hiding this comment

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

Got it, looks good then.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@jpkrohling
Copy link
Member

@bogdandrutu is there anything pending to merge this one?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@jpkrohling
Copy link
Member

@bogdandrutu , could you please merge this one?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 24, 2021
@bogdandrutu bogdandrutu removed the Stale label Sep 29, 2021
@bogdandrutu bogdandrutu merged commit 1fe1782 into open-telemetry:main Sep 30, 2021
luckyj5 pushed a commit to luckyj5/opentelemetry-collector-contrib that referenced this pull request Oct 1, 2021
luckyj5 pushed a commit to luckyj5/opentelemetry-collector-contrib that referenced this pull request Oct 1, 2021
luckyj5 added a commit to luckyj5/opentelemetry-collector-contrib that referenced this pull request Oct 1, 2021
```
time make -C ./receiver/opencensusreceiver test
make: Entering directory '/home/qmanfroi/git/signalfx/opentelemetry-collector-contrib/receiver/opencensusreceiver'
go test -race -timeout 300s --tags=containers_image_openpgp,exclude_graphdriver_btrfs,exclude_graphdriver_devicemapper ./...
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/opencensusreceiver   140.310s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/opencensusreceiver/internal/ocmetrics        0.329s
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/opencensusreceiver/internal/octrace  0.614s
make: Leaving directory '/home/qmanfroi/git/signalfx/opentelemetry-collector-contrib/receiver/opencensusreceiver'

real    2m21.503s
user    0m4.198s
sys     0m1.047s
```

here is the specification of my laptop:
* CPU: Intel(R) Core(TM) i7-8550U CPU
* Memory: 16GB
* Disk: PM981 NVMe Samsung 512GB
* OS: Arch Linux

* Update dependencies logzio exporter

* Updated example

* Updated README.md

* New line

* Update config.yaml

Co-authored-by: Bogdan Drutu <lazy@splunk.com>

Update loki-label-best-practices page (open-telemetry#5277)

[awscontainerinsights receiver]Add Readme for ECS (open-telemetry#4375)

* Add Readme for ECS

* Revise based on the comments

Add invert match on tail sampling string attribute (open-telemetry#4393)

Fix $attributes.file_path when collecting logs from Kubernetes (open-telemetry#5061)

* Update otel-collector-config.yml

On AWS EKS 1.21, I'd get this error (from your original code):

```console
$ kubectl logs -f --tail=10 ds/otel-collector

2021-08-28T17:46:38.930Z	error	Failed to process entry	{"kind": "receiver", "name": "filelog", "operator_id": "$.extract_metadata_from_filepath", "operator_type": "regex_parser", "error": {"description": "Entry is missing the expected parse_from field.", "suggestion": "Ensure that all incoming entries contain the parse_from field.", "details": {"parse_from": "$attributes.file_path"}}, "action": "send", "entry": {"timestamp":"2021-08-28T17:22:14.809709717Z","body":{"log":"2021-08-28T17:22:14+0000 DEBUG This is a debug log that shows a log that can be ignored.\n","stream":"stdout"},"attributes":{"file.path":"/var/log/pods/default_random-logger_4cf99717-08db-425b-88b6-1ee98fdf3660/random-logger/0.log"},"severity":0}}
```

The error message is showing that the actual attribute is "file.path", not "file_path". My proposed fix will correct that.

* Update otel-collector.yaml

* support untyped and _total metrics without the suffix in the metadata lookup

* Fix spacing

* remove debug line and run go fmt

* fix typo

* Add Apache httpd receiver - part 1

* Incorporate PR feedback

* Remove unnecessary string building step

* Simplify config validation

* Merge main

* Do not require a path

* Remove port requirement

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

* Add mysql receiver config and factory

* Incorporate PR feedback

* Add dependabot

* Rerun make generate

* PR feedback

Bump gopsutil/v3 to v3.21.9 (open-telemetry#5544)

* Bump gopsutil/v3

* make go tidy

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

bump aws-sdk-go to v1.40.53 (open-telemetry#5569)

fix: remove squash on configtls.TLSClientSetting for AWS components (open-telemetry#5454)

* fix: remove squash on configtls.TLSClientSetting

* fix: update yamls and readmes to reflect de-squashed tls

* fix: solve merge conflict

* fix: remove unnecessary file changes

* chore: add CHANGELOG

* fix: add relevant readme files

* fix: includes aws components only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants