Change otlp attribute conversion to be consistent with prometheus #6272
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SungJin1212
force-pushed
the
Change-otlp-handler-to-consist-with-prometheus
branch
from
a42d813 to
633baf5
Compare
|
@yeya24 |
yeya24
reviewed
Oct 19, 2024
CHANGELOG.md
Outdated
| @@ -9,6 +9,7 @@ | |||
| * [FEATURE] Ruler: Minimize chances of missed rule group evaluations that can occur due to OOM kills, bad underlying nodes, or due to an unhealthy ruler that appears in the ring as healthy. This feature is enabled via `-ruler.enable-ha-evaluation` flag. #6129 | |||
| * [FEATURE] Store Gateway: Add an in-memory chunk cache. #6245 | |||
| * [FEATURE] Chunk Cache: Support multi level cache and add metrics. #6249 | |||
| * [ENHANCEMENT] OTLP: Change otlp handler to consist with the Prometheus otlp handler. #6272 | |||
There was a problem hiding this comment.
Let's be more specific about the change. I can see 3 main changes:
- By default we don't convert all attributes to labels. This is a change
- Target info will be behind a FF. Enabled by default.
- New config to specify
promote_resource_attributesper tenant
|
@yeya24 |
|
I think it needs to be consistent with the disable convert all attributes flag. If that flag is not per tenant then target info shouldn't be per tenant. If our intended behavior is to be consistent with Prometheus then I think it makes more sense to be global and not per tenant. Only resource attribute promotion is per tenant and it should be only valid if all attributes conversion is disabled |
SungJin1212
force-pushed
the
Change-otlp-handler-to-consist-with-prometheus
branch
3 times, most recently
from
77f9e92 to
878f14e
Compare
yeya24
changed the title
SungJin1212
force-pushed
the
Change-otlp-handler-to-consist-with-prometheus
branch
2 times, most recently
from
3bc34b2 to
0230822
Compare
yeya24
approved these changes
Nov 4, 2024
pkg/distributor/distributor.go
Outdated
| @@ -188,6 +196,9 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | |||
|
|
|||
| f.Float64Var(&cfg.InstanceLimits.MaxIngestionRate, "distributor.instance-limits.max-ingestion-rate", 0, "Max ingestion rate (samples/sec) that this distributor will accept. This limit is per-distributor, not per-tenant. Additional push requests will be rejected. Current ingestion rate is computed as exponentially weighted moving average, updated every second. 0 = unlimited.") | |||
| f.IntVar(&cfg.InstanceLimits.MaxInflightPushRequests, "distributor.instance-limits.max-inflight-push-requests", 0, "Max inflight push requests that this distributor can handle. This limit is per-distributor, not per-tenant. Additional requests will be rejected. 0 = unlimited.") | |||
|
|
|||
| f.BoolVar(&cfg.OTLPConfig.ConvertAllAttributes, "distributor.otlp-config.convert-all-attributes", false, "If enabled, all resource attributes are converted to labels.") | |||
There was a problem hiding this comment.
Can we just simplify the flag name to be distributor.otlp.xxx?
pkg/distributor/distributor.go
Outdated
| @@ -188,6 +196,9 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | |||
|
|
|||
| f.Float64Var(&cfg.InstanceLimits.MaxIngestionRate, "distributor.instance-limits.max-ingestion-rate", 0, "Max ingestion rate (samples/sec) that this distributor will accept. This limit is per-distributor, not per-tenant. Additional push requests will be rejected. Current ingestion rate is computed as exponentially weighted moving average, updated every second. 0 = unlimited.") | |||
| f.IntVar(&cfg.InstanceLimits.MaxInflightPushRequests, "distributor.instance-limits.max-inflight-push-requests", 0, "Max inflight push requests that this distributor can handle. This limit is per-distributor, not per-tenant. Additional requests will be rejected. 0 = unlimited.") | |||
|
|
|||
| f.BoolVar(&cfg.OTLPConfig.ConvertAllAttributes, "distributor.otlp-config.convert-all-attributes", false, "If enabled, all resource attributes are converted to labels.") | |||
| f.BoolVar(&cfg.OTLPConfig.DisableTargetInfo, "distributor.otlp-config.disable-target-info", false, "If enabled, a target_info metric is not ingested.") | |||
There was a problem hiding this comment.
How about If true, target_info metric is not ingested.?
If enabled might be a bit confusing to users.
I suggest we describe what target_info metric does or put some relevant links to Opentelemetry
There was a problem hiding this comment.
Yes, Adding a link explaining what target_info is would be good.
yeya24
reviewed
Nov 4, 2024
CHANGELOG.md
Outdated
| @@ -12,6 +12,7 @@ | |||
| * [ENHANCEMENT] S3 Bucket Client: Add a list objects version configs to configure list api object version. #6280 | |||
| * [ENHANCEMENT] Query Frontend: Add new query stats metrics `cortex_query_samples_scanned_total` and `cortex_query_peak_samples` to track scannedSamples and peakSample per user. #6228 | |||
| * [ENHANCEMENT] Ingester: Disable chunk trimming. #6270 | |||
| * [ENHANCEMENT] OTLP: Change otlp handler to consist with the Prometheus otlp handler. Enable `target_info` metric and disable convert all attributes to labels by default. Add a config to specify promote resource attributes for tenants. #6272 | |||
There was a problem hiding this comment.
This should be a change instead of an enhancement.
SungJin1212
force-pushed
the
Change-otlp-handler-to-consist-with-prometheus
branch
from
0230822 to
2942e00
Compare
|
@yeya24 |
SungJin1212
force-pushed
the
Change-otlp-handler-to-consist-with-prometheus
branch
5 times, most recently
from
6f53a03 to
c04f607
Compare
yeya24
approved these changes
Nov 5, 2024
SungJin1212
force-pushed
the
Change-otlp-handler-to-consist-with-prometheus
branch
2 times, most recently
from
18dad00 to
4f43226
Compare
Signed-off-by: SungJin1212 <[email protected]>
SungJin1212
force-pushed
the
Change-otlp-handler-to-consist-with-prometheus
branch
from
4f43226 to
fcd9f51
Compare
CharlieTLe
pushed a commit
to CharlieTLe/cortex
that referenced
this pull request
Dec 3, 2024
…rtexproject#6272) Signed-off-by: SungJin1212 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change otlp handler to consist with the prometheus otlp handler.
Changes are as follows:
target_infometric by default, it is only disabled whendistributor.otlp-config.disable-target-infoistrue.-distributor.otlp-config.convert-all-attributesflag to retain the existing behavior (always convert all metric attributes)distributor.promote-resource-attributesto specify promte resource attributes for each tenants. It works only ifdistributor.otlp-config.convert-all-attributesisfalse.Which issue(s) this PR fixes:
Fixes #6236
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]