Add cortex otlp converter #7014
Conversation
SungJin1212
force-pushed
the
Add-cortex-otlp-converter
branch
4 times, most recently
from
cce44e2 to
a33b4a8
Compare
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
SungJin1212
force-pushed
the
Add-cortex-otlp-converter
branch
from
a33b4a8 to
efb1c0d
Compare
There was a problem hiding this comment.
Thanks for the work. The benchmark results seem promising.
However, I am a bit concerned about all the code we have to copy and vendor ourselves. It seems not easy to maintain and keep it sync with upstream. And there is no tests associated.
I feel there are better ways to allow customizing the OTLP converter for Cortex structs. Maybe we can work with upstream community to expose certain common functions we need to avoid duplicating almost 100% of the upstream OTLP converter logic.
There was a problem hiding this comment.
Nit. I would remove cortex prefix for the package. We can just have a dedicated otlp package and have a converter subpackage or the same package
| errs = multierr.Append(errs, fmt.Errorf("empty data points. %s is dropped", metric.Name())) | ||
| break | ||
| } | ||
| if settings.ConvertHistogramsToNHCB { |
There was a problem hiding this comment.
I think we don't support NHCB today via PRWv1. Maybe I am wrong. At least there is no test today to verify its support in Cortex.
For PRWv2 we can support NHCB I guess. But do we have test cases for that?
|
|
||
| // map ensures no duplicate label names. | ||
| l := make(map[string]string, maxLabelCount) | ||
| labelNamer := otlptranslator.LabelNamer{UTF8Allowed: settings.AllowUTF8} |
There was a problem hiding this comment.
Cortex still doesn't support UTF-8 names.
|
|
||
| type Settings struct { | ||
| Namespace string | ||
| ExternalLabels map[string]string |
There was a problem hiding this comment.
What's the usecase of Namespace and ExternalLabels? Do we have those configs before in Cortex? If there is no such config before, I recommend removing them if there is no such usecase
| ExternalLabels map[string]string | ||
| DisableTargetInfo bool | ||
| AddMetricSuffixes bool | ||
| AllowUTF8 bool |
There was a problem hiding this comment.
Cortex doesn't support UTF-8 today
| LookbackDelta time.Duration | ||
| // PromoteScopeMetadata controls whether to promote OTel scope metadata to metric labels. | ||
| PromoteScopeMetadata bool | ||
| EnableTypeAndUnitLabels bool |
There was a problem hiding this comment.
We should clean up the settings a little bit. If there are options that are not in Cortex for now, let's not include them in this PR
|
@yeya24 The upstream exposes a |
This PR introduces a dedicated Cortex OTLP converter that directly converts OTLP to Cortex format. It can skip the intermediate conversion from Prometheus format to Cortex for performance.
The converter codes are ported from the prometheus(https://github.com/prometheus/prometheus/tree/main/storage/remote/otlptranslator/prometheusremotewrite)
The old benchmark result: #6977
The new benchmark result:
The comparison result:
Which issue(s) this PR fixes:
Fixes #7002
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]