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

Log Support in Attribute Processor (2/2) #1830

Closed
wants to merge 1 commit into from
Closed

Log Support in Attribute Processor (2/2) #1830

wants to merge 1 commit into from

Conversation

keitwb
Copy link
Contributor

@keitwb keitwb commented Sep 22, 2020

This complements the trace span support already present.

It shares the same config struct to maintain backwards-compatibility with spans in the cleanest way possible.

Ignore the first commit, it is in PR #1783.

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #1830 into master will decrease coverage by 0.04%.
The diff coverage is 91.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1830      +/-   ##
==========================================
- Coverage   91.36%   91.32%   -0.05%     
==========================================
  Files         280      278       -2     
  Lines       16640    16535     -105     
==========================================
- Hits        15203    15100     -103     
+ Misses       1006     1003       -3     
- Partials      431      432       +1     
Impacted Files Coverage Δ
processor/spanprocessor/span.go 88.76% <ø> (ø)
processor/attributesprocessor/factory.go 82.60% <80.95%> (-2.58%) ⬇️
processor/attributesprocessor/attributes_log.go 100.00% <100.00%> (ø)
processor/attributesprocessor/attributes_trace.go 92.85% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 89.36% <0.00%> (-2.42%) ⬇️
processor/filterprocessor/filter_processor.go 88.88% <0.00%> (-1.59%) ⬇️
...mplingprocessor/tailsamplingprocessor/processor.go 74.03% <0.00%> (-0.37%) ⬇️
service/telemetry.go 81.96% <0.00%> (-0.30%) ⬇️
service/service.go 52.67% <0.00%> (-0.05%) ⬇️
consumer/pdata/trace.go 100.00% <0.00%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68085ca...a4c63fb. Read the comment docs.

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Sep 24, 2020

BTW, I was playing with it, checking if it can extract information from the standard log file name format in K8s:

  attributes:
    actions:
      - key: fluent.tag
        action: extract
        pattern: (?P<pod_name>[a-z0-9](?:[-a-z0-9]*[a-z0-9])?(?:\.[a-z0-9](?:[-a-z0-9]*[a-z0-9])?)*)_(?P<namespace_name>[^_]+)_(?P<container_name>.+)-(?P<docker_id>[a-z0-9]{64})\.log$

And it worked out of the box:

     -> fluent.tag: STRING(collection-kube-state-metrics-bff7f888d-wbd5c_test_kube-state-metrics-1c03625d46cdf4f23a727a737867db7df8b2fdd4f1308d3e0d618f6ec89df462.log)
     -> pod_name: STRING(collection-kube-state-metrics-bff7f888d-wbd5c)
     -> namespace_name: STRING(test)
     -> container_name: STRING(kube-state-metrics)
     -> docker_id: STRING(1c03625d46cdf4f23a727a737867db7df8b2fdd4f1308d3e0d618f6ec89df462)

Copy link
Contributor

@ccaraman ccaraman left a comment

Choose a reason for hiding this comment

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

LGTM - In addition to the comments/making code coverage happy, please update:

  1. https://github.com/open-telemetry/opentelemetry-collector/blob/master/processor/attributesprocessor/testdata/config.yaml to include examples for logs
  2. Update the readme to reflect it working on logs format too.

processor/attributesprocessor/attributes_log.go Outdated Show resolved Hide resolved
processor/attributesprocessor/attributes_log.go Outdated Show resolved Hide resolved
processor/attributesprocessor/attributes_log.go Outdated Show resolved Hide resolved
processor/attributesprocessor/attributes_log_test.go Outdated Show resolved Hide resolved
processor/attributesprocessor/attributes_log_test.go Outdated Show resolved Hide resolved
This complements the trace span support already present.

It shares the same config struct to maintain backwards-compatibility with spans in the cleanest way possible.
@keitwb
Copy link
Contributor Author

keitwb commented Oct 7, 2020

ok @ccaraman copy/paste stuff should be fixed now.

@zeitlinger
Copy link
Member

@keitwb there is a merge conflict now, because log filtering got more powerful in #928 and wants to have a reference to the resource and library as well.

Since there is a lot of code re-use between span and log filtering now, the tests for log filtering can be greatly reduced.

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Oct 9, 2020

Since there is a lot of code re-use between span and log filtering now, the tests for log filtering can be greatly reduced.

BTW, I think there's a room to generalise the code for testing logs/metrics/trace processors. One example in which I did it is here, though I guess there might be better ways to do it. Perhaps it should be extracted as a some sort of helper (just thinking out loud)

@zeitlinger
Copy link
Member

@keitwb I've resolved the merge conflict in #1934

@zeitlinger
Copy link
Member

Since there is a lot of code re-use between span and log filtering now, the tests for log filtering can be greatly reduced.

BTW, I think there's a room to generalise the code for testing logs/metrics/trace processors. One example in which I did it is here, though I guess there might be better ways to do it. Perhaps it should be extracted as a some sort of helper (just thinking out loud)

just tried this out in #1934 - I think it looks readable without any helper methods - just using table tests.

@keitwb
Copy link
Contributor Author

keitwb commented Oct 12, 2020

CLosing in favor of #1934.

@keitwb keitwb closed this Oct 12, 2020
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* Remove bundler from Jaeger exporter

* Update dependencies

* Add changes to changelog

* Add PR number to changelog
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
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.

7 participants