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 support for Prometheus' start time metric #394

Merged
merged 4 commits into from
Oct 16, 2019
Merged

Add support for Prometheus' start time metric #394

merged 4 commits into from
Oct 16, 2019

Conversation

dinooliva
Copy link
Contributor

Prometheus clients can optionally export a standard start-time gauge metric, process_start_time_seconds, that greatly simplifies the logic necessary for setting the start timestamp for exported timeseries, which removes the overhead of the metrics adjuster.

This change adds a configuration option to use the start-time metric. If the start-time metric is not found in a scrape, the timeseries for that scrape are dropped.

Note that the start-time timeseries is not exported by the prometheus receiver as a metric (i.e. not passed to ConsumeMetricsData).

@codecov-io
Copy link

codecov-io commented Oct 11, 2019

Codecov Report

Merging #394 into master will decrease coverage by 0.23%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
- Coverage   74.46%   74.22%   -0.24%     
==========================================
  Files         116      116              
  Lines        6997     7026      +29     
==========================================
+ Hits         5210     5215       +5     
- Misses       1527     1549      +22     
- Partials      260      262       +2
Impacted Files Coverage Δ
receiver/prometheusreceiver/metrics_receiver.go 70.17% <100%> (+1.08%) ⬆️
receiver/prometheusreceiver/internal/ocastore.go 100% <100%> (ø) ⬆️
...eceiver/prometheusreceiver/internal/transaction.go 64.13% <32.25%> (-18.73%) ⬇️
...iver/prometheusreceiver/internal/metricsbuilder.go 97.91% <70%> (-2.09%) ⬇️

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 54ecd58...251b01c. Read the comment docs.

@dinooliva
Copy link
Contributor Author

@rghetia - ptal

Copy link
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

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

LGTM except for one minor nit.

droppedTimeseries = numTimeseries
} else {
adjustStartTime(tr.metricBuilder.startTime, metrics)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, it would be more readable to have following

	if tr.useStartTimeMetric {
 		// adjustStartTime
	} else {
		// AdjustMetrics. jobsMap has to be non-nil in this case.
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM. One related q but not from your change.

droppedTimeseries int
useStartTimeMetric bool
startTime float64
logger *zap.SugaredLogger
Copy link
Contributor

Choose a reason for hiding this comment

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

Q.: this is not a change but typically we use *zap.Logger. Are you aware of any specific reason to use the SugaredLogger? The former is strong typed and as result a bit more verbose on the source but the rest of the code base already uses it....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why the original author used SugaredLogger but I don't see any particular need for it - I can write a follow-up pr to remove it.

@pjanotti pjanotti merged commit 2ed85b2 into open-telemetry:master Oct 16, 2019
@dinooliva dinooliva deleted the prom-start-time branch October 16, 2019 22:51
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024
* add OpenTelemetry demo example

* Update Makefile

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
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.

4 participants