-
Notifications
You must be signed in to change notification settings - Fork 893
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
Add FaaS metrics semantic conventions #1736
Conversation
@skonto The namespace/prefix is shared with the trace and resource semantic conventions: If you think changing it would make sense for all three of them, please open a separate issue to discuss this. |
Yes that is why I set this PR to wip, wanted see if I can do it here, but I agree better to start a discussion first. |
Question for maintainers. What are the expectations around duplication of data, or preferences for where it resides? Taking a specific example, the |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@arminru gentle ping for review. |
@kenfinnigan |
Would I also wasn't sure whether there would be overlap between |
@kenfinnigan In the trace semantic conventions things are quite clear since incoming and outgoing invocations are distinguished clearly. In tracing, @skonto I think it would make sense to use the same structure as in the trace semantic conventions (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/faas.md) here as well to make this clearer. |
@arminru Thanks for the detail. The piece I was missing is that metrics could have an equivalent "in process" and "outgoing" as traces, as I'd presumed metrics would only be dealing with the current process only |
@arminru Yes I can do that
I thought the same but then it might be the case that we have a metric that counts invocations from within a process |
@arminru I updated the spec, I dont want to repeat the info at the trace side (added a link), however I changed the structure according to other metrics specs. |
Ok I will update this. |
@arminru I updated the instrument types, regarding the other update you mention, I suspect the resource labels can be mixed in with the ones in this PR if they have to, I am not sure if I need to reference them though at all. |
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.
@skonto Thanks! Seems like the PR I referred to only added to the FaaS trace semconv but did not touch the attributes which you took over, so no need for updating then. The resource attributes don't have to recited here, yes.
@open-telemetry/specs-metrics-approvers Please take a look.
This comment has been minimized.
This comment has been minimized.
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.
Thanks, @skonto!
@open-telemetry/specs-metrics-approvers Please take a look and review/approve. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@jmacd gentle ping, can I get merge pls? |
* Define FaaS Metric Semantics Closes open-telemetry#1013 * adjust faas metrics * fixes * docs * updates * fixes * update instruments * use attributes Co-authored-by: Michael Lavers <[email protected]> Co-authored-by: Carlos Alberto Cortez <[email protected]>
Fixes #1013
Changes
Adds metrics and metric labels for functions
I am proposing also changing the
faas
prefix tofunc
as not all environments are of that kind.Do I need to discuss that change in a WG meeting?
/cc @kolanos @kenfinnigan @jmacd