receiver/prometheus: wire up direct pdata conversion mechanism by Aneurysm9 · Pull Request #6421 · open-telemetry/opentelemetry-collector-contrib

Aneurysm9 · 2021-11-23T23:24:56Z

Description: Adds the remaining pieces of a direct-to-pdata mechanism for the Prometheus receiver and a feature gate that allows selecting whether to use the legacy OpenCensus translations or the new direct route.

Link to tracking Issue: #4892

Testing: Unit tests exist for new code in the internal package. End-to-end tests for the receiver now run against configurations that use both the OpenCensus and pdata builders.

…thout OpenCensus Wire up and use the direct Prometheus->Pdata conversion end to end. With this change the receiver will no longer need OpenCensus. This change will involve more follow-ups that just migrate over the tests, because we don't want a super bloated/massive PR. Fixes open-telemetry#4892 Depends on PR open-telemetry/opentelemetry-collector#3694 Depends on PR open-telemetry/opentelemetry-collector#3695

…ireUp-roundTrip

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…ut-of-order exposition Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…ireUp-roundTrip

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…ensus or pdata directly Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

… issues with out-of-order exposition data Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…port paths Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

… copy of opencensus translator during development Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Aneurysm9 · 2021-11-23T23:26:52Z

@dashpole can you take a look at this?

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…tor-contrib into receiver-prometheus-wireUp-roundTrip

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Aneurysm9 · 2021-11-24T01:26:30Z

Changes outside of receiver/prometheusreceiver are due to #6305 adding a CI check using porto but not ensuring that it was up-to-date with HEAD. If another PR that has to make the same changes to pass the lint check lands first then they should disappear from this PR.

… suite runtime Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

dashpole · 2021-11-24T17:14:44Z

 	BufferCount             int                      `mapstructure:"buffer_count"`
 	UseStartTimeMetric      bool                     `mapstructure:"use_start_time_metric"`
 	StartTimeMetricRegex    string                   `mapstructure:"start_time_metric_regex"`
+	pdataDirect             bool


It is probably simpler just to use featuregate.IsEnabled(pdataPipelineGate.ID) where it is needed, rather than store it here. Is there a way to set feature gates during a test?

There is only a global feature gate registry, so it can't necessarily be used in tests. The feature gate registry also has a lock, so we recommend that the feature status is checked once and stored if it is needed repeatedly. There is no mechanism for operators to change feature gate statuses during program runtime, so it should be safe to read this once and re-use it.

…ing test suite runtime" This reverts commit 7e04b44.

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…tor-contrib into receiver-prometheus-wireUp-roundTrip Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…tor-contrib into receiver-prometheus-wireUp-roundTrip

dashpole

looks great!

dashpole · 2021-11-29T19:46:26Z

Huh, i'm sure why my approval checkmark isn't green. I'm in the code owners entry for the prometheus receiver

Aneurysm9 · 2021-11-29T20:01:22Z

Huh, i'm sure why my approval checkmark isn't green. I'm in the code owners entry for the prometheus receiver

Not sure what's going on there. It doesn't look like there are any files outside /receiver/prometheus/ so you should be an owner for all files in the review.

In any event, this should have more eyes on it. @anuraaga and @codeboten can I get a review?

…tor-contrib into receiver-prometheus-wireUp-roundTrip

Aneurysm9 · 2021-11-29T20:10:35Z

@dashpole apparently to be an effective code owner for review approval purposes you must have write access to the repository:

The people you choose as code owners must have write permissions for the repository.

About CODEOWNERS

Though that seems to be contradicted later in the same document:

A CODEOWNERS file uses a pattern that follows most of the same rules used in gitignore files, with some exceptions. The pattern is followed by one or more GitHub usernames or team names using the standard @username or @org/team-name format. Users must have read access to the repository and teams must have explicit write access, even if the team's members already have access.

The same document (emphasis in original)

v0v

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…tor-contrib into receiver-prometheus-wireUp-roundTrip

anuraaga

Intense PR did a first lightweight pass. I definitely wasn't expecting this much logic to convert between two metrics formats 😮

High level question, is this embedding functionality from cumulativetodelta? And if so, should it be?

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/cumulativetodeltaprocessor/internal/tracking/metric.go

Also, things like metrics GC feel like they'd already be in the OTel SDK. Curious if there's no way to leverage the OTel SDK to do this conversion instead of what may be a lot of similar concepts. I wouldn't imagine there to be a huge penalty in efficiency, but not sure. And also just a wild idea that came to mind, it may not be applicable at all.

anuraaga · 2021-11-30T04:06:57Z

+	histogram.SetAggregationTemporality(pdata.MetricAggregationTemporalityCumulative)
+
+	destPointL := histogram.DataPoints()
+	// By default the AggregationTemporality is Cumulative until it'll be changed by the caller.


From what I can tell this is referring to line 53

I think this comment is either a vestige of a prior pdata API or a misunderstanding by the original author. The default aggregation temporality appears to be unspecified. See https://github.com/open-telemetry/opentelemetry-collector/blob/main/model/pdata/metrics.go#L256-L261. I am removing this comment.

anuraaga · 2021-11-30T04:09:28Z

+
+func gaugeDistMetricPdata(name string, kvp []*kv, startTs pdata.Timestamp, points ...*pdata.HistogramDataPoint) *pdata.Metric {
+	hMetric := cumulativeDistMetricPdata(name, kvp, startTs, points...)
+	hMetric.Histogram().SetAggregationTemporality(pdata.MetricAggregationTemporalityDelta)


From what I understand, all prometheus data is cumulative including gauge, but am likely wrong.

Wondering if there is a reference for the mapping, I searched around and found this doc with a broken link to a design doc. Would help reviewing this PR if there's information about the mapping somewhere

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/openmetrics-guidelines.md

I'm not actually sure. This is inherited test helper code that seems to be used to test adjustment of gauge histograms, which should never be encountered since the appender won't build them and the metric adjuster will short circuit when encountering a distribution with other than cumulative temporality. I think this can be removed without impacting functionality or test coverage.

anuraaga · 2021-11-30T04:15:53Z

+// Get the timeseriesinfo for the timeseries associated with the metric and label values.
+func (tsm *timeseriesMapPdata) get(metric *pdata.Metric, kv pdata.AttributeMap) *timeseriesinfoPdata {
+	name := metric.Name()
+	sig := getTimeseriesSignaturePdata(name, kv)


Maybe better to use a hashed object instead of string for map key

I think the cloudwatch exporter has a pattern using a Sorted object that could work

I'm not sure I follow. The pdata.AttributeMap isn't comparable and can't be used as a map key. Since the labels are user-provided and arbitrary I'm not sure we could create a comparable object other than a string from them.

There's this attribute.Distinct type used here for something similar I think recommended by jmacd a while back

opentelemetry-collector-contrib/internal/aws/metrics/metric_calculator.go

Line 90 in 243d642

MetricLabels attribute.Distinct

I figure it has at least somewhat better performance than strings

That is from the Go SDK's attribute package, which is not in use here. These are pdata.AttributeMaps which would require similar conversion to utilize attribute.Distinct.

anuraaga · 2021-11-30T04:18:04Z

+		tsi = &timeseriesinfoPdata{}
+		tsm.tsiMap[sig] = tsi
+	}
+	tsm.mark = true


Since gc() uses a lock to access tsi, I'd expect a lock here too, or otherwise atomic write. If it's already locked at a higher level, then a comment could help

The lock is taken in *MetricsAdjusterPdata.AdjustMetrics(), which is the first exported method encountered when walking up the call stack from all invocations of this method. This is the same as the OpenCensus-based implementation in metrics_adjuster.go that has been in use by this receiver for a long time. I expect that to be the case for most of the comments on this implementation as most of them are in repeated code.

A reworking of this implementation is probably in order, particularly given the issue noted by @dashpole regarding targets that are scraped less frequently than the fixed GC interval. That should probably be handled in a separate issue.

anuraaga · 2021-11-30T04:21:53Z

+	jm.RLock()
+	defer jm.RUnlock()
+	if time.Since(jm.lastGC) > jm.gcInterval {
+		go jm.gc()


If get can be called from multiple threads, I think this can cause gc to be called multiple times concurrently, I suspect that's not intended. Can use a comment if this case can't happen since a simple method like get we'd generally expect to not have any threading guarantees

The time since the last GC is checked again inside gc() once a lock is obtained. It may be triggered multiple times, but it will still only run once per interval. That's what the comments in this method and in gc() are trying to convey.

anuraaga · 2021-11-30T04:22:29Z

+}
+
+func (jm *JobsMapPdata) get(job, instance string) *timeseriesMapPdata {
+	sig := job + ":" + instance


Notice above using printf and here using concatenation, I think most code in collector would use printf

anuraaga · 2021-11-30T04:24:15Z

+// Returns the total number of timeseries that had reset start times.
+func (ma *MetricsAdjusterPdata) AdjustMetrics(metricL *pdata.MetricSlice) int {
+	resets := 0
+	ma.tsm.Lock()


It's somewhat mysterious we're locking on tsm here but not accessing it (presumably we are accessing through adjustMetric). Maybe a comment to ensure it was intended

anuraaga · 2021-11-30T04:24:43Z

+
+	default:
+		// this shouldn't happen
+		ma.logger.Info("Adjust - skipping unexpected point", zap.String("type", dataType.String()))


Debug level probably

This is currently info in the OC metrics adjuster. If anything, since this is a situation that should never occur I might want to make this a warning.

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

…tor-contrib into receiver-prometheus-wireUp-roundTrip

…' into receiver-prometheus-wireUp-roundTrip

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Aneurysm9 · 2021-12-03T16:23:49Z

@open-telemetry/collector-contrib-approvers can I get some more eyes on this? We have further work that is held up behind getting these changes landed. Thanks!

alolita · 2021-12-03T17:14:19Z

Given @Aneurysm9 and @dashpole are codeowners for this component, can we get this PR merged please? Can the permissions be fixed @bogdandrutu @codeboten

There are other PRs (14 PRs - 9 tests and 5 bug fixes) that are blocked on this merge.

bogdandrutu · 2021-12-03T18:51:06Z

@alolita I am not OK to merge this since a very good engineer and approver reviewed this (@anuraaga) and comments are not addressed.

…tor-contrib into receiver-prometheus-wireUp-roundTrip

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

anuraaga

I hadn't realized at first that there isn't that much new code here, a lot is copied from the old translation path to allow a feature flag deprecation. So generally good with the code as it seems to be a relatively simple change on top of that - would appreciate some more code comments to verify lock expectations though

…tor-contrib into receiver-prometheus-wireUp-roundTrip

…djuster. Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

CatherineF-dev · 2022-08-03T12:27:25Z

Hi, thanks for the great work which possibly reduces open-telemetry memory to 50% in a testing cluster. (v0.41 v.s. v0.40)

Want to cherry-pick this magic into previous versions. Do we know which small change in this PR is related to reduce agent memory?

dashpole · 2022-08-03T14:21:04Z

@CatherineF-dev I'm afraid this isn't an easily cherry-pickable change, as it involved refactoring the entire receiver.

…ess (#6421) Unrevert #6267 and make it so that we never error out. Co-authored-by: Alex Boten <alex@boten.ca>

odeke-em and others added 16 commits September 20, 2021 23:35

Retrofit top level package for pdata comparisons

5cf4213

Reuse for pdata metrics creators from PR open-telemetry#5231

959a3ed

Shed all in-code direct references to OpenCensus

de159f6

Merge remote-tracking branch 'origin/main' into receiver-prometheus-w…

a90ea33

…ireUp-roundTrip

receiver/prometheus: fix tests

20bf876

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: buffer metrics in builder to avoid issues with o…

0874f0f

…ut-of-order exposition Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Merge remote-tracking branch 'origin/main' into receiver-prometheus-w…

4739bcf

…ireUp-roundTrip

receiver/prometheus: restore prom->OC->pdata pipeline

1c85958

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: add feature gate to control whether to use OpenC…

65fd778

…ensus or pdata directly Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: localize metrics test helpers

6770a1c

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: store metrics in a map in metricbuilder to avoid…

9208e41

… issues with out-of-order exposition data Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: run component tests against both OC and pdata ex…

8845e18

…port paths Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: ensure OC metrics builder tests are deterministic

e710275

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: restore replace directive to ensure use of local…

5a56ab7

… copy of opencensus translator during development Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: fix lint issues

f862b94

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Aneurysm9 requested review from a team and dmitryax November 23, 2021 23:24

github-actions Bot assigned kbrockhoff Nov 23, 2021

receiver/prometheus: fix lint issues

598f4a2

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Aneurysm9 force-pushed the receiver-prometheus-wireUp-roundTrip branch from 8382077 to 598f4a2 Compare November 24, 2021 00:19

Aneurysm9 added 2 commits November 23, 2021 20:16

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

2112e47

…tor-contrib into receiver-prometheus-wireUp-roundTrip

make porto

838f7a5

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

receiver/prometheus: run e2e tests in parallel to avoid doubling test…

7e04b44

… suite runtime Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

dashpole reviewed Nov 24, 2021

View reviewed changes

Aneurysm9 added 3 commits November 24, 2021 13:23

Revert "receiver/prometheus: run e2e tests in parallel to avoid doubl…

d3ff42a

…ing test suite runtime" This reverts commit 7e04b44.

receiver/prometheus: address PR feedback

4fb78ab

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

f293579

…tor-contrib into receiver-prometheus-wireUp-roundTrip Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

29dfb74

…tor-contrib into receiver-prometheus-wireUp-roundTrip

dashpole approved these changes Nov 29, 2021

View reviewed changes

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

ae0559b

…tor-contrib into receiver-prometheus-wireUp-roundTrip

Aneurysm9 added 2 commits November 29, 2021 15:52

make porto

d0bd35d

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

4922d5c

…tor-contrib into receiver-prometheus-wireUp-roundTrip

anuraaga reviewed Nov 30, 2021

View reviewed changes

Aneurysm9 added 4 commits November 30, 2021 00:24

receiver/prometheus: remove gauge histogram pdata adjustment tests

5f52842

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

589a21d

…tor-contrib into receiver-prometheus-wireUp-roundTrip

Merge remote-tracking branch 'a9/receiver-prometheus-wireUp-roundTrip…

ad029fa

…' into receiver-prometheus-wireUp-roundTrip

receiver/prometheus: remove unused gauge histogram data generator helper

24060cf

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

vishiy approved these changes Dec 1, 2021

View reviewed changes

alolita added the ready to merge Code review completed; ready to merge by maintainers label Dec 3, 2021

bogdandrutu removed the ready to merge Code review completed; ready to merge by maintainers label Dec 3, 2021

Aneurysm9 added 2 commits December 6, 2021 16:53

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

4fce276

…tor-contrib into receiver-prometheus-wireUp-roundTrip

Fix lint errors

9344131

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

anuraaga approved these changes Dec 7, 2021

View reviewed changes

Aneurysm9 added 2 commits December 7, 2021 15:47

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

5c67cc5

…tor-contrib into receiver-prometheus-wireUp-roundTrip

receiver/prometheus: Add comments detailing use of locks in metrics a…

9ed90fe

…djuster. Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

Aneurysm9 added the ready to merge Code review completed; ready to merge by maintainers label Dec 8, 2021

bogdandrutu merged commit 22d15bb into open-telemetry:main Dec 8, 2021

Aneurysm9 deleted the receiver-prometheus-wireUp-roundTrip branch December 8, 2021 18:52

Conversation

Aneurysm9 commented Nov 23, 2021

Uh oh!

Aneurysm9 commented Nov 23, 2021

Uh oh!

Aneurysm9 commented Nov 24, 2021

Uh oh!

dashpole Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dashpole left a comment

Choose a reason for hiding this comment

Uh oh!

dashpole commented Nov 29, 2021

Uh oh!

Aneurysm9 commented Nov 29, 2021

Uh oh!

Aneurysm9 commented Nov 29, 2021

Uh oh!

anuraaga left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aneurysm9 Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aneurysm9 commented Dec 3, 2021

Uh oh!

alolita commented Dec 3, 2021

Uh oh!

bogdandrutu commented Dec 3, 2021

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

CatherineF-dev commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

dashpole Nov 24, 2021 •

edited

Loading

anuraaga left a comment •

edited

Loading

Aneurysm9 Nov 30, 2021 •

edited

Loading

CatherineF-dev commented Aug 3, 2022 •

edited

Loading