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

Adds a new config StartTimeMetricRegex #1511

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Aug 6, 2020

Previously the prometheus receiver only accepts
process_start_time_metric as the start time when UseStartTimeMetric is
set. For applications that export similar metrics but with a prefix,
e.g., via NewProcessCollector(namespace), the receiver would drop such
metrics.
By adding StartTimeMetricRegex, at least we will be able to cover such
use cases.

related: #969

Testing: Adds new unit tests and end-to-end test case in prometheus receiver.

Documentation: none.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 6, 2020

CLA Check
The committers are authorized under a signed CLA.

@project-bot project-bot bot added this to In progress in Collector Aug 6, 2020
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 6, 2020

PTAL @nilebox :)

Copy link
Member

@nilebox nilebox left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor comments.

receiver/prometheusreceiver/config_test.go Outdated Show resolved Hide resolved
receiver/prometheusreceiver/internal/metricsbuilder.go Outdated Show resolved Hide resolved
receiver/prometheusreceiver/metrics_receiver_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #1511 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1511   +/-   ##
=======================================
  Coverage   91.35%   91.36%           
=======================================
  Files         240      240           
  Lines       16744    16754   +10     
=======================================
+ Hits        15297    15307   +10     
  Misses       1044     1044           
  Partials      403      403           
Impacted Files Coverage Δ
...iver/prometheusreceiver/internal/metricsbuilder.go 100.00% <100.00%> (ø)
receiver/prometheusreceiver/internal/ocastore.go 93.75% <100.00%> (+0.20%) ⬆️
...eceiver/prometheusreceiver/internal/transaction.go 95.00% <100.00%> (+0.05%) ⬆️
receiver/prometheusreceiver/metrics_receiver.go 79.54% <100.00%> (ø)

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 9e70411...6da2d55. Read the comment docs.

Collector automation moved this from In progress to Reviewer approved Aug 7, 2020
@nilebox
Copy link
Member

nilebox commented Aug 7, 2020

@bogdandrutu
Copy link
Member

Lint errors still present.

Previously the prometheus receiver only accepts
`process_start_time_metric` as the start time when UseStartTimeMetric is
set. For applications that export similar metrics but with a prefix,
e.g., via NewProcessCollector(namespace), the receiver would drop such
metrics.
By adding StartTimeMetricRegex, at least we will be able to cover such
use cases.
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 7, 2020

PTAL

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 7, 2020

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Aug 10, 2020

@nilebox @bogdandrutu PTAL, thanks!

@bogdandrutu bogdandrutu merged commit c74b42d into open-telemetry:master Aug 10, 2020
Collector automation moved this from Reviewer approved to Done Aug 10, 2020
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
 (open-telemetry#1511)

Co-authored-by: Lalit Kumar Bhasin <labhas@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants