Prometheus exporter: concurrent collect bugfix

[hexdigest]

This PR fixes the following problem which is caused by the concurrent Collect method execution:

fatal error: concurrent map writes

goroutine 112 [running]:
go.opentelemetry.io/otel/exporters/prometheus.validateMetrics({0xc0004fc1e0, 0x28}, {0x0, 0x0}, 0xc00024e4c8, 0xc000122008?)
        /go/pkg/mod/go.opentelemetry.io/otel/exporters/[email protected]/exporter.go:384 +0x5fd
go.opentelemetry.io/otel/exporters/prometheus.addSumMetric[...](0x7f27d20dd108?, {{0xc0003693b0?, 0x40f7c7?, 0x28?}, 0x80?, 0xb8?}, {{0x1390626, 0x22}, {0x0, 0x0}, ...}, ...)
        /go/pkg/mod/go.opentelemetry.io/otel/exporters/[email protected]/exporter.go:214 +0xcd
go.opentelemetry.io/otel/exporters/prometheus.(*collector).Collect(0xc00009e730, 0xc0004f5f60?)
        /go/pkg/mod/go.opentelemetry.io/otel/exporters/[email protected]/exporter.go:160 +0xc6e
github.com/prometheus/client_golang/prometheus.(*Registry).Gather.func1()

Also fixes #3917

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: hexdigest / name: Max Chechel (1a30f23, 4e06c3e, 24e2472, e382059, befb8c2, 675c0fb, 162e9fb, ce2045f, 3483cb8, 77959ad, c00462c, 96f7fd2, 1f7c3ad, ecc1336, ea67c1a, b4b3c2a, c274d95, 782f393, 489b528, 35809a9)
• ✅ login: pellared / name: Robert Pająk (bb27c28, 4c03f86, 529e868, 489b528, 1eed860, af3d7cf, 919987c, c85e301, ec77a14, 87f5902, 60ce2f0, d272f93, fcfb775, e70b79f, 4b5714d, 914a931)
• ✅ login: dmathieu / name: Damien Mathieu (7b284eb, 2a69d79)
• ✅ login: dashpole / name: David Ashpole (4b5714d)

[MrAlias]

This is missing tests to validate the bug this is supposed to address is resolved.

[Aneurysm9]

Is this type expected to be safe for concurrent use? I don't believe we document that any exporters are safe for concurrent use. What is the scenario that causes this error to be observed?

[hexdigest]

Is this type expected to be safe for concurrent use? I don't believe we document that any exporters are safe for concurrent use.

Neither prometheus nor opentelemetry explicitly documented anything about concurrency:

https://github.com/prometheus/client_golang/blob/27f0506d6ebbb117b6b697d0552ee5be2502c5f2/prometheus/registry.go#L140:L160

What is the scenario that causes this error to be observed?

We use OpenTelemetry with Prometheus in an absolutely standard way. I think that it just happen that collector came concurrently which is very unlikely but still possible.

[hexdigest]

@MrAlias @Aneurysm9

Digging deeper I found out that concurrent map access isn't the only problem, there are other critical sections. So I cleaned up the code and used sync.Mutex instead of just protecting the map. Please take a look at the PR and if it's ok I'll complete it with tests.

This particular collector must be concurrent safe just because there's no mechanism protecting the third-party collector on the Prometheus side, as result a couple of concurrent requests to the prometheus handler might crash the app.

In the Prometheus itself the metrics map is protected by the mutex:
https://github.com/prometheus/client_golang/blob/fae2f6306b16a7265d5569c5507623dfaba09053/prometheus/vec.go#L320:L323

So any Prometheus collector must be concurrent-safe.

[codecov]

Codecov Report

Merging #3899 (914a931) into main (64e76f8) will decrease coverage by 0.1%.
The diff coverage is 74.3%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #3899     +/-   ##
=======================================
- Coverage   83.6%   83.6%   -0.1%     
=======================================
  Files        181     181             
  Lines      14162   14184     +22     
=======================================
+ Hits       11840   11858     +18     
- Misses      2096    2098      +2     
- Partials     226     228      +2     

Impacted Files	Coverage Δ
exporters/prometheus/exporter.go	84.4% <74.3%> (+0.5%)	⬆️

... and 1 file with indirect coverage changes

[MrAlias]

This is still missing regression tests.

[hexdigest]

This is still missing regression tests.

@MrAlias I added a test for concurrent collect. When I ran it against the main branch it gave me exactly same panic I reported above:

WARNING: DATA RACE
Read at 0x00c00051a880 by goroutine 125:
  github.com/prometheus/client_model/go.(*MetricFamily).GetType()
      /home/max/go/pkg/mod/github.com/prometheus/[email protected]/go/metrics.pb.go:823 +0xcf
  go.opentelemetry.io/otel/exporters/prometheus.validateMetrics()
      /tmp/opentelemetry-go/exporters/prometheus/exporter.go:394 +0xdf
  go.opentelemetry.io/otel/exporters/prometheus.addSumMetric[...]()
      /tmp/opentelemetry-go/exporters/prometheus/exporter.go:217 +0x113
  go.opentelemetry.io/otel/exporters/prometheus.(*collector).Collect()
      /tmp/opentelemetry-go/exporters/prometheus/exporter.go:163 +0x1711
  go.opentelemetry.io/otel/exporters/prometheus.TestConcurrentCollect.func1()
      /tmp/opentelemetry-go/exporters/prometheus/exporter_test.go:712 +0x3e

Previous write at 0x00c00051a880 by goroutine 123:
  go.opentelemetry.io/otel/exporters/prometheus.validateMetrics()
      /tmp/opentelemetry-go/exporters/prometheus/exporter.go:387 +0x956
  go.opentelemetry.io/otel/exporters/prometheus.addSumMetric[...]()
      /tmp/opentelemetry-go/exporters/prometheus/exporter.go:217 +0x113
  go.opentelemetry.io/otel/exporters/prometheus.(*collector).Collect()
      /tmp/opentelemetry-go/exporters/prometheus/exporter.go:163 +0x1711
  go.opentelemetry.io/otel/exporters/prometheus.TestConcurrentCollect.func1()
      /tmp/opentelemetry-go/exporters/prometheus/exporter_test.go:712 +0x3e

PTAL

[hexdigest]

Just curious how many more months it will take to fix several panics in this package.

[pellared]

Just curious how many more months it will take to fix several panics in this package.

@hexdigest I am planning to merge this today as there was enough time for other contributors to review. Can you just update the TestNotNilScopeinfoInCollect test failure which is caused by recent changes (#4210)?

[dashpole]

Sorry I wasn't able to get to this sooner, i'm just getting back from leave. This is a great improvement overall, and a needed fix. Comments are non-blocking.

[pellared]

@hexdigest Are you able to fix the build and consider addressing @dashpole comments?

[hexdigest]

@hexdigest Are you able to fix the build and consider addressing @dashpole comments?

Yep, I will

[hexdigest]

@hexdigest Are you able to fix the build and consider addressing @dashpole comments?

The problem is that this is the only trick path I found to reproduce the issue in the test:
invalid instrument name: ���: must start with a letter

And now it's being checked somewhere else I suppose, so I might just remove this test.

[pellared]

I am fine with removing the test as it is a very edge case and personally I do not know how to reproduce it.

[pellared]

@hexdigest I quickly checked https://github.com/prometheus/client_golang/ and tried to fix the test in browser. fcfb775

I hope it helps...

I am quite busy at the moment 😬

[pellared]

@hexdigest Let us know if you do not have time or do not want to address the comments. I can merge this PR and address the comments myself by creating a new PR.

[hexdigest]

Let's merge it now

…

On Thu, Jun 29, 2023, 11:47 Robert Pająk ***@***.***> wrote: @hexdigest <https://github.com/hexdigest> Let us know if you do not have time or do not want to address the comments. I can merge this PR and address the comments myself by creating a new PR. — Reply to this email directly, view it on GitHub <#3899 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALEA6BK3WWBEBG4TCJ4IMLXNUXHRANCNFSM6AAAAAAV6PG2JQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>