Fix data-race in metric without `code` and `method` but with `WithLabelFromCtx` #1318

tigrato · 2023-07-30T20:29:34Z

This PR fixes a data race that exists when the metric used in any promhttp middleware doesn't collect the code and method but uses WithLabelFromCtx to collect values from context.

The problem happens because when no code and method tags are collected, the labels function returns a pre-initialized map emptyLabels for every request.

When one or multiple WithLabelFromCtx options are configured, the returned map from the labels function call is used to collect the metrics from context which creates a multi-write data race.

Fixes #1317

tigrato · 2023-07-30T20:30:55Z

Bartłomiej Płotka @bwplotka
Kemal Akkoyun @kakkoyun

Do you mind taking a look?

bwplotka

Thanks! Can we add regression test? This will make sure we don't reintroduce same bug.

Current logic with emptyLabels that at some point are not empty is also weird, let's fix it.

bwplotka · 2023-07-31T09:47:27Z

prometheus/promhttp/instrument_server.go

+		if hasExtraLabelsCtx {
+			return prometheus.Labels{}
+		}
 		return emptyLabels


This feels wrong. If the emptyLabels are no longer empty, this is highly misleading. We should fix the code and not rely on that global var or make sure no one writes to it.

Thank you @bwplotka

I removed the usage of the global emptyLabels map because as you say it's highly misleading and it makes it trivial to introduce new data races by writing into the output of the labels func.

…omCtx` This commit fixes a data race that exists when the metric used in any `promhttp` middleware doesn't collect the `code` and `method` but uses `WithLabelFromCtx` to collect values from context. The problem happens because when no `code` and `method` tags are collected, the `labels` function returns a pre-initialized map `emptyLabels` for every request. When one or multipe `WithLabelFromCtx` options are configured, the returned map from the `labels` function call is used to collect the metrics from context which creates a multi-write data race. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>

bwplotka

Wow, that was simple fix =D

bwplotka reviewed Jul 31, 2023

View reviewed changes

tigrato force-pushed the fix-data-race branch from f488fe2 to 5113fb6 Compare July 31, 2023 09:59

tigrato requested a review from bwplotka July 31, 2023 10:03

bwplotka approved these changes Aug 1, 2023

View reviewed changes

bwplotka merged commit 59c00e3 into prometheus:main Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix data-race in metric without `code` and `method` but with `WithLabelFromCtx` #1318

Fix data-race in metric without `code` and `method` but with `WithLabelFromCtx` #1318

tigrato commented Jul 30, 2023

tigrato commented Jul 30, 2023

bwplotka left a comment

bwplotka Jul 31, 2023

tigrato Jul 31, 2023

bwplotka left a comment

Fix data-race in metric without code and method but with WithLabelFromCtx #1318

Fix data-race in metric without code and method but with WithLabelFromCtx #1318

Conversation

tigrato commented Jul 30, 2023

tigrato commented Jul 30, 2023

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Jul 31, 2023

Choose a reason for hiding this comment

tigrato Jul 31, 2023

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Fix data-race in metric without `code` and `method` but with `WithLabelFromCtx` #1318

Fix data-race in metric without `code` and `method` but with `WithLabelFromCtx` #1318