OTLP: add support for counters and gauges #133702
Conversation
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
elasticsearchmachine
added
Team:StorageEngine
v9.2.0
external-contributor
| ObjectPath search = ObjectPath.createFromResponse( | ||
| client().performRequest(new Request("GET", "metrics-generic.otel-default" + "/_search")) | ||
| ); | ||
| assertThat(search.evaluate("hits.total.value"), equalTo(1)); |
There was a problem hiding this comment.
Can't we validate the response here as in the next one?
| // If the processing of the request fails, | ||
| // the server MUST respond with appropriate HTTP 4xx or HTTP 5xx status code. | ||
| // https://opentelemetry.io/docs/specs/otlp/#failures-1 | ||
| status = RestStatus.INTERNAL_SERVER_ERROR; |
There was a problem hiding this comment.
Should we be checking the returned errors in the responses? For instance, if they all report bad argument, this should be BAD_REQUEST?
There was a problem hiding this comment.
I'm wondering whether I misinterpreted the spec here. Maybe we should also return 200, similar to what we do in the _bulk API, no matter of one, almost all, or all individual items have failed. We then rely on the rejected data points and the error message to convey that all items have failed.
| // If the processing of the request fails, | ||
| // the server MUST respond with appropriate HTTP 4xx or HTTP 5xx status code. | ||
| new MetricsResponse( | ||
| RestStatus.INTERNAL_SERVER_ERROR, |
There was a problem hiding this comment.
@martijnvg I don't think we want to return 500 here?
There was a problem hiding this comment.
Which return code would be more appropriate here?
...lugin/otel-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPoint.java
Show resolved
Hide resolved
...lugin/otel-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPoint.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPointGroupingContext.java
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPointGroupingContext.java
Outdated
Show resolved
Hide resolved
|
Let's split the data point and tsid logic to separate PRs and add unit tests. |
...lugin/otel-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPoint.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Kostas Krikellas <[email protected]>
I've reverted the changes to the action classes and created unit tests for grouping and dynamic templates. Please have another look. I'll follow up with the transport/rest action changes. I like that the test coverage is ensure more by unit tests rather than integration tests. This makes debugging and test execution a lot easier. I'll still add more rest tests but I suppose they can be a bit lighter on test coverage where covering the edge cases is handled by the unit tests. |
x-pack/plugin/otel-data/src/test/java/org/elasticsearch/xpack/oteldata/otlp/OtlpUtils.java
Outdated
Show resolved
Hide resolved
...test/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPointGroupingContextTests.java
Outdated
Show resolved
Hide resolved
...test/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPointGroupingContextTests.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/elasticsearch/xpack/oteldata/otlp/proto/BufferedByteStringAccessorTests.java
Outdated
Show resolved
Hide resolved
This is in line with that the ES exporter is doing and these fields aren't mapped as a dimension anyway
Part of #133057