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

internal/splunk: fallback to host if cloud resource id not found #1170

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

jrcamp
Copy link
Contributor

@jrcamp jrcamp commented Oct 1, 2020

APM correlation will need to do same thing so add fallback logic in common.

@jrcamp jrcamp requested a review from dmitryax October 1, 2020 20:29
@jrcamp jrcamp requested a review from a team as a code owner October 1, 2020 20:29
@project-bot project-bot bot added this to In progress in Collector Oct 1, 2020
@jrcamp jrcamp marked this pull request as draft October 1, 2020 20:30
@jrcamp jrcamp marked this pull request as ready for review October 1, 2020 20:36
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

splunk is the only vendor specific thing in common anymore, would you consider moving that to internal/splunk please?

go.mod Outdated Show resolved Hide resolved
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

Collector automation moved this from In progress to Reviewer approved Oct 1, 2020
@jrcamp
Copy link
Contributor Author

jrcamp commented Oct 1, 2020

splunk is the only vendor specific thing in common anymore, would you consider moving that to internal/splunk please?

I will do in followup PR. I should make internal/splunk its own go module I assume?

@tigrannajaryan
Copy link
Member

Build failed.

@bogdandrutu
Copy link
Member

Please rebase

APM correlation will need to do same thing so add fallback logic in common.
@codecov
Copy link

codecov bot commented Oct 5, 2020

Codecov Report

Merging #1170 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1170      +/-   ##
==========================================
- Coverage   89.82%   89.77%   -0.05%     
==========================================
  Files         285      285              
  Lines       13882    13878       -4     
==========================================
- Hits        12469    12459      -10     
- Misses       1044     1052       +8     
+ Partials      369      367       -2     
Flag Coverage Δ
#integration 75.68% <0.00%> (-0.33%) ⬇️
#unit 88.93% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
exporter/signalfxexporter/hostmetadata/metadata.go 100.00% <100.00%> (ø)
internal/common/splunk/hostid.go 100.00% <100.00%> (ø)
receiver/redisreceiver/interval/interval_runner.go 21.05% <0.00%> (-21.06%) ⬇️
receiver/k8sclusterreceiver/watcher.go 95.29% <0.00%> (-2.36%) ⬇️

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 727b1b6...07b53ec. Read the comment docs.

@jrcamp
Copy link
Contributor Author

jrcamp commented Oct 5, 2020

@tigrannajaryan fixed

@tigrannajaryan tigrannajaryan merged commit 0689ef6 into open-telemetry:master Oct 5, 2020
Collector automation moved this from Reviewer approved to Done Oct 5, 2020
@jrcamp jrcamp deleted the hostid branch October 9, 2020 16:43
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
Note: All of the cudos go to @dneray for the logic/testing/effort in open-telemetry/opentelemetry-collector#981

This pr is based off of it with the following modifications

- The config looks like
```
- key : <key to use for applying the rule too>
  pattern: <the regex pattern with named submatchers>
  action: extract
```
I think that the original PR is going in the right way for what we can the internal logic to be but as the issue #979 and #1170 indicate we need to take a step back and redesign the attributes processor (or make it clear of its limitations).  This pr unblocks the required functionality. 

**Link to tracking Issue:** #979 

**Link to follow up issue:** #1170 

**Testing:** Added tests for extracting.

**Documentation:** Updated processor readme.
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
…#1170)

* Don't consider unset env var an error during detection

* update CHANGELOG
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
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants