-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Add OTLP metrics endpoint #133057
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add OTLP metrics endpoint #133057
Conversation
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
...gin/otel-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/OTLPMetricsRestAction.java
Show resolved
Hide resolved
Did a quick test, and so far seems to be working as expected. For everyone that needs a config to do a quick test run with the otel collector:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a look at the histogram stuff only
...l-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/HistogramConverter.java
Outdated
Show resolved
Hide resolved
...l-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/HistogramConverter.java
Outdated
Show resolved
Hide resolved
@ParametersFactory(argumentFormatting = "%1$s") | ||
public static List<Object[]> testCases() { | ||
return List.of( | ||
new Object[] { "empty", HistogramDataPoint.newBuilder().build(), List.of(), List.of() }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to my reading of the spec it should be legal to have a single count and no bucket boundaries. This implies a single (-INF, +INF)
bucket, in which case we should (and I think do) return an empty histogram.
Maybe add that case too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's illegal according to the spec we could also return a validation error and drop it.
335ce54
to
e6dc2db
Compare
@@ -59,6 +59,7 @@ dependencies { | |||
// utilities | |||
api project(":libs:cli") | |||
implementation 'com.carrotsearch:hppc:0.8.1' | |||
api 'com.dynatrace.hash4j:hash4j:0.25.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be adding any dependencies to server. This needs to be hidden, so as not to collide with any plugins that may try to use (a different version) of this dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. I'm currently looking at the impact of not using this library in favor of the existing murmur implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a good solution to use the existing hasher without regressions: #133226
Adds an OTLP endpoint for metric ingestion into TSDB.
This simplifies getting data into TSDB as configuration of mappings and dimensions isn't required. It also improves ingestion performance. Partly due to the binary encoding but also because it can re-use partial
TsidBuilder
s for common resource attributes.My aim was to retain exactly the same semantics and behavior as the OTel collector's Elasticsearch exporter, including data stream routing and mapping hints.
Depends on
To review, select from the 3rd commit (9cea250) onwards
Note that I might force-push to the branch once the PRs this depends on get merged.