Skip to content

[ML] Inference duration and error metrics #115876

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 5, 2024
Merged

[ML] Inference duration and error metrics #115876

merged 5 commits into from
Nov 5, 2024

Conversation

prwhelan
Copy link
Member

Add es.inference.requests.time metric around infer API.

As recommended by OTel spec, errors are determined by the presence or absence of the error.type attribute in the metric. "error.type" will be the http status code (as a string) if it is available, otherwise it will be the name of the exception (e.g. NullPointerException).

Additional notes:

  • ApmInferenceStats is merged into InferenceStats. Originally we planned to have multiple implementations, but now we're only using APM.
  • Request count is now always recorded, even when there are failures loading the endpoint configuration.
  • Added a hook in streaming for cancel messages, so we can close the metrics when a user cancels the stream.

Example from local node to APM (redacted a bunch):

{ "_index": ".ds-metrics-apm.app.elasticsearch-default-2024.10.25-000001", "data_stream": { "dataset": "apm.app.elasticsearch", "namespace": "default", "type": "metrics" }, "es": { "inference": { "requests": { "time": { "values": [ 6992.5 ], "counts": [ 1 ] } } } }, "labels": { "model_id": "gpt-4o-mini", "otel_instrumentation_scope_name": "elasticsearch", "service": "openai", "task_type": "completion" }, "numeric_labels": { "status_code": 200 }, ... } }
@prwhelan
[ML] Inference duration and error metrics
c83298d 
Add `es.inference.requests.time` metric around `infer` API. As recommended by OTel spec, errors are determined by the presence or absence of the `error.type` attribute in the metric. "error.type" will be the http status code (as a string) if it is available, otherwise it will be the name of the exception (e.g. NullPointerException). Additional notes: - ApmInferenceStats is merged into InferenceStats. Originally we planned to have multiple implementations, but now we're only using APM. - Request count is now always recorded, even when there are failures loading the endpoint configuration. - Added a hook in streaming for cancel messages, so we can close the metrics when a user cancels the stream.
@prwhelan prwhelan added >enhancement :ml Machine learning Team:ML Meta label for the ML team v9.0.0 labels Oct 29, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @prwhelan, I've created a changelog YAML for you.
@prwhelan
Copy link
Member Author

@elasticmachine update branch
@prwhelan prwhelan marked this pull request as ready for review October 31, 2024 12:34
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)
maxhniebergall
maxhniebergall reviewed Oct 31, 2024
View reviewed changes
Copy link
Contributor

@maxhniebergall maxhniebergall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple of questions
...ference/src/main/java/org/elasticsearch/xpack/inference/action/TransportInferenceAction.java Outdated
Comment on lines 128 to 129
listener.onFailure(e);
recordMetrics(model, timer, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to do listenener.onFailure(e); after we recordMetrics. I don't think that any code after calling listener.on is guaranteed to run.
...in/inference/src/main/java/org/elasticsearch/xpack/inference/common/DelegatingProcessor.java
};
}

protected void onCancel() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why this is not an abstract method? Do we not want to implement this in all subclasses?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most subclasses don't have any actions to take when the stream is canceled, they're mostly just forwarding the request/cancel and modifying the response payload, so I figured we should just continue to no-op them for onCancel
maxhniebergall
maxhniebergall approved these changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@maxhniebergall maxhniebergall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@prwhelan
Copy link
Member Author

prwhelan commented Nov 5, 2024

@elasticmachine update branch
@prwhelan prwhelan merged commit 26870ef into elastic:main Nov 5, 2024
16 checks passed
jozala pushed a commit that referenced this pull request Nov 13, 2024
@prwhelan @jozala
[ML] Inference duration and error metrics (#115876)
e4f937f 
Add `es.inference.requests.time` metric around `infer` API. As recommended by OTel spec, errors are determined by the presence or absence of the `error.type` attribute in the metric. "error.type" will be the http status code (as a string) if it is available, otherwise it will be the name of the exception (e.g. NullPointerException). Additional notes: - ApmInferenceStats is merged into InferenceStats. Originally we planned to have multiple implementations, but now we're only using APM. - Request count is now always recorded, even when there are failures loading the endpoint configuration. - Added a hook in streaming for cancel messages, so we can close the metrics when a user cancels the stream.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
@prwhelan @alexey-ivanov-es
[ML] Inference duration and error metrics (elastic#115876)
eaffa1c 
Add `es.inference.requests.time` metric around `infer` API. As recommended by OTel spec, errors are determined by the presence or absence of the `error.type` attribute in the metric. "error.type" will be the http status code (as a string) if it is available, otherwise it will be the name of the exception (e.g. NullPointerException). Additional notes: - ApmInferenceStats is merged into InferenceStats. Originally we planned to have multiple implementations, but now we're only using APM. - Request count is now always recorded, even when there are failures loading the endpoint configuration. - Added a hook in streaming for cancel messages, so we can close the metrics when a user cancels the stream.
@jonathan-buttner
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Questions ?

Please refer to the Backport tool documentation
@jonathan-buttner jonathan-buttner mentioned this pull request Dec 13, 2024
Merged
jonathan-buttner pushed a commit to jonathan-buttner/elasticsearch that referenced this pull request Dec 13, 2024
@prwhelan
[ML] Inference duration and error metrics (elastic#115876)
575ece4 
Add `es.inference.requests.time` metric around `infer` API. As recommended by OTel spec, errors are determined by the presence or absence of the `error.type` attribute in the metric. "error.type" will be the http status code (as a string) if it is available, otherwise it will be the name of the exception (e.g. NullPointerException). Additional notes: - ApmInferenceStats is merged into InferenceStats. Originally we planned to have multiple implementations, but now we're only using APM. - Request count is now always recorded, even when there are failures loading the endpoint configuration. - Added a hook in streaming for cancel messages, so we can close the metrics when a user cancels the stream. (cherry picked from commit 26870ef)
maxhniebergall pushed a commit that referenced this pull request Dec 13, 2024
@jonathan-buttner @prwhelan
[8.x] [ML] Inference duration and error metrics (#115876) (#118700)
7eaf380 
* [ML] Inference duration and error metrics (#115876) Add `es.inference.requests.time` metric around `infer` API. As recommended by OTel spec, errors are determined by the presence or absence of the `error.type` attribute in the metric. "error.type" will be the http status code (as a string) if it is available, otherwise it will be the name of the exception (e.g. NullPointerException). Additional notes: - ApmInferenceStats is merged into InferenceStats. Originally we planned to have multiple implementations, but now we're only using APM. - Request count is now always recorded, even when there are failures loading the endpoint configuration. - Added a hook in streaming for cancel messages, so we can close the metrics when a user cancels the stream. (cherry picked from commit 26870ef) * fixing switch with class issue --------- Co-authored-by: Pat Whelan <pat.whelan@elastic.co>
maxhniebergall pushed a commit to maxhniebergall/elasticsearch that referenced this pull request Dec 16, 2024
@jonathan-buttner @prwhelan
[8.x] [ML] Inference duration and error metrics (elastic#115876) (ela…
8d694f9 
…stic#118700) * [ML] Inference duration and error metrics (elastic#115876) Add `es.inference.requests.time` metric around `infer` API. As recommended by OTel spec, errors are determined by the presence or absence of the `error.type` attribute in the metric. "error.type" will be the http status code (as a string) if it is available, otherwise it will be the name of the exception (e.g. NullPointerException). Additional notes: - ApmInferenceStats is merged into InferenceStats. Originally we planned to have multiple implementations, but now we're only using APM. - Request count is now always recorded, even when there are failures loading the endpoint configuration. - Added a hook in streaming for cancel messages, so we can close the metrics when a user cancels the stream. (cherry picked from commit 26870ef) * fixing switch with class issue --------- Co-authored-by: Pat Whelan <pat.whelan@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :ml Machine learning Team:ML Meta label for the ML team v9.0.0

5 participants

@prwhelan @elasticsearchmachine @jonathan-buttner @maxhniebergall @elasticmachine