Reimplement prometheus receiver #572

fivesheep · 2019-06-12T23:07:19Z

Problems with the current implementation of prometheus receiver

The current prometheus receiver is based on the https://github.com/orijtech/promreceiver library. in the past couple of months, we have found a couple of issues with this implementation and has reported either in gitter or the library's git repo, which including:

dose not support all the data types prometheus: Support Summary type. orijtech/promreceiver#5
throwing error when encounter unknown metrics type which caused the rest of the metric points from the prometheus scrape query to be lost: Do not drop all metrics when seeing unsupported types. orijtech/promreceiver#3
metric labels not handle properly: Metrics Labels are not converted properly orijtech/promreceiver#1
did not handle NaN values properly: NaN value not value not filtered orijtech/promreceiver#2

Why reimplementing instead of submitting fixes

So far, some of the above issues were fixed, some not. However, this is not the whole story. After diving deeper into the code, we have found that the the library actually has a number of serious bugs, we conclude that without a complete rewrite, it's not going to work properly:

A global appender is used, which can easily run into race conditions when multiple scrape target is configured
Failed to group metrics in the original form, such as a single group of metric from prometheus can be divided into multiple. This is a big issue if one wants to config a prometheus exporter on the other end of the agent to a prometheus server, as metric group with same name is not allowed.
The library is trying to cache all the data scraped from remote metrics endpoints, then feed the delta to downstream, this doesn't seem like what is expected. even worst, it has used a global cache to store the data, which can be easily corrupted when scraping multiple targets
The library is also trying to use the cache to generate STDDEV for histogram which is a mission impossible as the original data doesn't provide any original sampled points.

More details for this PR trying to address can be found in the following document, which is also part of the PR: https://github.com/fivesheep/opencensus-service/blob/reimplement-prometheus-receiver/receiver/prometheusreceiver/README.md

an example of issues (NaN) values:

with the following ocagent config

receivers:
    prometheus:
        config:
            global:
                scrape_interval: 120s
                scrape_timeout: 8s
            scrape_configs:
            - job_name: 'cadvisor'
              scrape_interval: 15s
              static_configs:
              - targets: ['localhost:8888']

exporters:
    prometheus:
            namespace: "promdemo"
            address: "localhost:8889"

compare the outputs from original prometheus endpoint and the ocagent prometheus exporter

original (cadvisor)

...
# HELP process_resident_memory_bytes Resident memory size in bytes.
# TYPE process_resident_memory_bytes gauge
process_resident_memory_bytes 5.6446976e+07
# HELP process_start_time_seconds Start time of the process since unix epoch in seconds.
# TYPE process_start_time_seconds gauge
process_start_time_seconds 1.56037276274e+09
# HELP process_virtual_memory_bytes Virtual memory size in bytes.
# TYPE process_virtual_memory_bytes gauge
process_virtual_memory_bytes 1.501503488e+09

from ocagent promethues exporter

...
# HELP promdemo_process_resident_memory Resident memory size in bytes.
# TYPE promdemo_process_resident_memory counter
promdemo_process_resident_memory NaN
# HELP promdemo_process_start_time Start time of the process since unix epoch in seconds.
# TYPE promdemo_process_start_time counter
promdemo_process_start_time NaN
# HELP promdemo_process_virtual_memory Virtual memory size in bytes.
# TYPE promdemo_process_virtual_memory counter
promdemo_process_virtual_memory NaN

codecov · 2019-06-13T17:06:03Z

Codecov Report

Merging #572 into master will increase coverage by 2.03%.
The diff coverage is 94.78%.

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
+ Coverage   66.41%   68.45%   +2.03%     
==========================================
  Files          86       91       +5     
  Lines        5518     5934     +416     
==========================================
+ Hits         3665     4062     +397     
- Misses       1645     1660      +15     
- Partials      208      212       +4

Impacted Files	Coverage Δ
...iver/prometheusreceiver/internal/metricsbuilder.go	`100% <100%> (ø)`
receiver/prometheusreceiver/internal/ocastore.go	`71.42% <71.42%> (ø)`
receiver/prometheusreceiver/metrics_receiver.go	`70.17% <74.19%> (-4.34%)`	⬇️
receiver/prometheusreceiver/internal/metadata.go	`75% <75%> (ø)`
receiver/prometheusreceiver/internal/logger.go	`77.77% <77.77%> (ø)`
...eceiver/prometheusreceiver/internal/transaction.go	`96.36% <96.36%> (ø)`
... and 1 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 d8a2872...4efb1f1. Read the comment docs.

pjanotti · 2019-06-13T21:41:05Z

Hi @fivesheep thanks for providing the context for this PR. It brings the whole functionality so it is on the larger side. We will need a bit of time to review all pieces, but, I will try to provide some of the feedback as we go.

songy23 · 2019-06-13T21:45:15Z

Agree this PR is a bit too large. Consider splitting it to multiple smaller PRs, like one for metricbuilder, one for logger, etc.

odeke-em · 2019-06-13T22:03:07Z

Thank you for working on this @fivesheep and for the fixes and overhaul! @pjanotti pinged me about it and my apologies for not being responsive on my repo as I've been swamped with work. I think that to get this implementation in here, some few suggestions:
a) @fivesheep perhaps you can put the new exporter under your repos, license it for OpenCensus Authors under Apache 2.0 for the time being and when @songy23 has finished creating a repository under https://github.com/census-ecosystem it can be migrated there and reviewed independently
b) Concurrently with step a) @songy23 please help create the vanity URL contrib.go.opencensus.io/receiver/prometheus and look at adding @fivesheep's exporter behind that
c) When https://github.com/census-ecosystem/opencensus-go-prometheus-receiver has been created, then you can start transferring the code piecemeal and have the respective small code reviews
d) When step c) is completed, the new receiver can reside behind the vanity URL and the repository from a) can be sunset.

I say this because the logic will take some time to review but during that this opencensus-service will have lots of changes in it. Working on it independently ensures you won't be blocked, you can iterate faster, make test suites etc but also that correctness supersedes :)

Hope that this can help with increasing the velocity of getting things in.

fivesheep · 2019-06-14T17:31:11Z

@pjanotti @songy23 and @odeke-em thanks for the response. I would love to create an individual repo to host the library, however, I cannot make this decision by myself because of my company's opensource policy. I would need to ask our opensource committee if I am allowed to do that, or I need to go through the other opensource request, which can take a very long time to get approval.

pjanotti · 2019-06-14T17:54:47Z

@fivesheep we would like to eventually make this part of the core as we are going to leverage OC service to be the starting point of https://github.com/open-telemetry/opentelemetry-service/tree/master/docs and Prometheus is part of the core. The contrib was proposed as a temporary stage to make things speedier meanwhile, if that is not the case then we make this work directly on core.

rghetia

minor comments so far. Still more to review.

rghetia · 2019-06-14T20:23:01Z

receiver/prometheusreceiver/README.md

+
+
+### Metric Value Mapping
+ In OpenCensus, metrics value types can be either `int64` or `float64`, while in in Prometheus the value can be safety assume it's always `float64` based on the 


nit: s/safety/safely/

rghetia · 2019-06-14T20:27:59Z

receiver/prometheusreceiver/README.md

+}
+```
+
+*Note: `tsOc` is a timestamp object representing the current ts*


StartTimestamp cannot be current timestamp for counters. It should be the timestamp when metric collection started.

You are right, the current ts here was actually referring to the same thing, I will update the description as per your comment to reduce the confusion.

I was trying to differentiate timestamps provided from the original metric endpoint and the ones generated by the scraperLoop which you were referring to, and I meant current.

An example of such metric output is shown below:

# HELP http_requests_total The total number of HTTP requests. # TYPE http_requests_total counter http_requests_total{method="post",code="200"} 1027 1395066363000 http_requests_total{method="post",code="400"} 3 1395066363000

once the honor_timestamps flag of scrapping config is set to true, which is the default value, this timestamp is respected, the logic can be found from: https://github.com/prometheus/prometheus/blob/0c0638b080cf1565fc4e5b7ee3fd35c36ae5832a/scrape/scrape.go#L1084-L1091

In the implantation of prometheus receiver, whichever timestamp is provided from the Add/AddFast method will be used.

receiver/prometheusreceiver/README.md

receiver/prometheusreceiver/internal/internal_test.go

receiver/prometheusreceiver/internal/metricsbuilder.go

receiver/prometheusreceiver/README.md

rghetia · 2019-06-16T14:06:41Z

receiver/prometheusreceiver/metrics_receiver_test.go

-			Type:        metricspb.MetricDescriptor_CUMULATIVE_DISTRIBUTION,
-			LabelKeys: []*metricspb.LabelKey{
-				{Key: "method"},
+	want2 := []data.MetricsData{


want2 and want1 are same.

rghetia · 2019-06-16T14:13:15Z

receiver/prometheusreceiver/metrics_receiver_test.go

+	for _, want := range wantPermutations {
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf("different metric got:\n%v\nwant:\n%v\n", string(exportertest.ToJSON(got)),
+				string(exportertest.ToJSON(want)))


is the purpose here to fail if either want1 or want2 doesn't match? (assuming that want1 and want2 are different. see prev comment. they don't seem to be different)

This test was inherited from the original implementation of pmreceiver, @odeke-em might have more insight on this test.

googlebot · 2019-06-18T04:15:28Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

…ing the sum/count from snapshot to global

…metheus-receiver # Conflicts: # go.mod

googlebot · 2019-06-18T04:35:45Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

dinooliva · 2019-06-21T18:23:45Z

receiver/prometheusreceiver/internal/metricsbuilder.go

+		groups := b.currentDpGroupOrdered()
+		if len(groups) == 0 {
+			// this can happen if only sum or count is added, but not following data points
+			return errors.New("no data point added to summary")


For Python, the Prometheus client code is incomplete for summaries and only exports sum and count - consider changing this to a log message and returning nil otherwise the entire scrape will be lost for any Python application with a Summary metric.

Good catch, I have observed similar behavior very recently from a MicroMeter' prometheus endpoint that a summary only contains sum and count.
Returning nil, as suggested, is one way to solve the issue, however, the down side is that we might loss some important metrics which customers are interested in. Or we can convert these kind of metrics differently into something like two counters, or dose OpenCensus format allow to have Summary Snapshots with no Percentile values?

Additionally, the Prometheus Go client now exports empty summaries by default.

I believe that exporting summary snapshot with empty percentile values makes the most sense.

@songy23 @rghetia - wdyt?

I agree with empty percentile values.

will do. I am also going to drop gaugehistogram support from this pr until an official spec of this data type is provided. as hint by @dinooliva , I have checked some code from the python prometheus client, and found it had some code related to this gaugehistogram type from this PR prometheus/client_python#306 , and it seems like its a bit different from regular histograms, instead of using _count/_sum it uses _gcount/_gsum as postfixes. the PR has also attached a draft spec link to openmetrics, the next of prometheus, which can be found from https://docs.google.com/document/d/1KwV0mAXwwbvvifBvDKH_LU1YjyXE_wxCkHNoCGq1GX0/edit#heading=h.1cvzqd4ksd23

it also has some interesting new features like _created to indicate the start time of metrics, however, this spec is still in draft stage, and I am not sure when it will be published.

dinooliva · 2019-06-24T19:29:57Z

receiver/prometheusreceiver/internal/transaction_test.go

+		}
+	})
+
+	t.Run("Rollback dose nothing", func(t *testing.T) {


dinooliva · 2019-06-24T19:42:58Z

receiver/prometheusreceiver/internal/ocastore_test.go

+`
+
+// https://github.com/prometheus/docs/blob/master/content/docs/instrumenting/exposition_formats.md#text-format-example
+var testData1 = `


Consider splitting the different metric types into different tests and add any missing metric types (e.g. gauge).

Also consider adding more edge cases (e.g. histogram with 1 bucket).

dinooliva · 2019-06-26T00:24:01Z

We'd like to use this code in a project that we're currently working on so I've created two issues (#588 and #589) for tracking the identified issues with the code.

We plan to merge the code and will fix the issues in subsequent pr's.

Reimplement prometheus receiver

76015a5

fivesheep requested review from pjanotti and a team as code owners June 12, 2019 23:07

googlebot added the cla: yes label Jun 12, 2019

fivesheep added 3 commits June 12, 2019 21:51

fix go vet failures

aa0bb14

go fmt

4636f89

fix golint

6962f57

songy23 requested a review from dinooliva June 13, 2019 21:31

rghetia reviewed Jun 14, 2019

View reviewed changes

rghetia reviewed Jun 16, 2019

View reviewed changes

fivesheep requested review from songy23 and tigrannajaryan as code owners June 18, 2019 04:15

googlebot added cla: no and removed cla: yes labels Jun 18, 2019

fivesheep added 2 commits June 17, 2019 21:33

update doc/code based on comment, also update the summary impl by mov…

6df375a

…ing the sum/count from snapshot to global

Merge remote-tracking branch 'opencensus/master' into reimplement-pro…

7a8d3c9

…metheus-receiver # Conflicts: # go.mod

fivesheep force-pushed the reimplement-prometheus-receiver branch from 0cc9c12 to 7a8d3c9 Compare June 18, 2019 04:35

googlebot added cla: yes and removed cla: no labels Jun 18, 2019

minor document update

2c827bc

dinooliva reviewed Jun 21, 2019

View reviewed changes

dinooliva reviewed Jun 24, 2019

View reviewed changes

This was referenced Jun 26, 2019

Prometheus Receiver: Initial time series values and resets not handle appropriately #588

Closed

Prometheus Receiver: Summaries with empty quantiles should map to empty percentiles #589

Open

Merge branch 'master' into reimplement-prometheus-receiver

4efb1f1

songy23 approved these changes Jun 26, 2019

View reviewed changes

bogdandrutu approved these changes Jun 26, 2019

View reviewed changes

songy23 merged commit efd4d2d into census-instrumentation:master Jun 26, 2019

songy23 mentioned this pull request Jun 26, 2019

Import the changes in Prometheus receiver from OpenCensus-Service. open-telemetry/opentelemetry-collector#60

Closed

dinooliva mentioned this pull request Jun 28, 2019

Imports new Prometheus receiver from OpenCensus. open-telemetry/opentelemetry-collector#65

Merged

flands assigned fivesheep Jun 28, 2019

flands added this to the 0.1.9 milestone Jun 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement prometheus receiver #572

Reimplement prometheus receiver #572

fivesheep commented Jun 12, 2019

codecov bot commented Jun 13, 2019 •

edited

Loading

pjanotti commented Jun 13, 2019

songy23 commented Jun 13, 2019

odeke-em commented Jun 13, 2019

fivesheep commented Jun 14, 2019

pjanotti commented Jun 14, 2019 •

edited

Loading

rghetia left a comment

rghetia Jun 14, 2019

rghetia Jun 14, 2019

fivesheep Jun 15, 2019

rghetia Jun 16, 2019

rghetia Jun 16, 2019

fivesheep Jun 18, 2019

googlebot commented Jun 18, 2019

googlebot commented Jun 18, 2019

dinooliva Jun 21, 2019

fivesheep Jun 21, 2019

dinooliva Jun 24, 2019

rghetia Jun 24, 2019

fivesheep Jun 25, 2019

dinooliva Jun 24, 2019

dinooliva Jun 24, 2019

dinooliva commented Jun 26, 2019



		### Metric Value Mapping
		In OpenCensus, metrics value types can be either `int64` or `float64`, while in in Prometheus the value can be safety assume it's always `float64` based on the

Reimplement prometheus receiver #572

Reimplement prometheus receiver #572

Conversation

fivesheep commented Jun 12, 2019

Problems with the current implementation of prometheus receiver

Why reimplementing instead of submitting fixes

an example of issues (NaN) values:

codecov bot commented Jun 13, 2019 • edited Loading

Codecov Report

pjanotti commented Jun 13, 2019

songy23 commented Jun 13, 2019

odeke-em commented Jun 13, 2019

fivesheep commented Jun 14, 2019

pjanotti commented Jun 14, 2019 • edited Loading

rghetia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 18, 2019

googlebot commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinooliva commented Jun 26, 2019

codecov bot commented Jun 13, 2019 •

edited

Loading

pjanotti commented Jun 14, 2019 •

edited

Loading