Skip to content
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

Generate FaaS metrics semconv from YAML #88

Merged

Conversation

joaopgrassi
Copy link
Member

@joaopgrassi joaopgrassi commented Jun 6, 2023

Closes #77, #86

CC @skonto @tylerbenson as previous authors

@joaopgrassi joaopgrassi force-pushed the feat/faas-metrics-yaml branch from 81e6e51 to 14ecb07 Compare July 6, 2023 12:29
@joaopgrassi joaopgrassi marked this pull request as ready for review July 6, 2023 12:30
@joaopgrassi joaopgrassi requested review from a team July 6, 2023 12:30
semantic_conventions/faas-common.yaml Outdated Show resolved Hide resolved
@joaopgrassi
Copy link
Member Author

This PR still suffers from the issues I described in #86. It's not clear which attributes are to be added to which metrics. There's seems to be a distinction between incoming and outgoing metrics/attributes but the current conventions don't distinguish them. Any people familiar/experienced with this topic that can help?

@joaopgrassi joaopgrassi force-pushed the feat/faas-metrics-yaml branch 2 times, most recently from c375c82 to cfe0e20 Compare July 7, 2023 07:05
@trask
Copy link
Member

trask commented Jul 17, 2023

@open-telemetry/lambda-extension-approvers can you review?

Copy link
Member

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Assuming this doesn't introduce anything new and is just moving existing metric semconv to YAML. (That seems to be the case, but difficult to tell.)

@joaopgrassi
Copy link
Member Author

@tylerbenson yes, that was the goal - just move them to yaml.

But these conventions I think are unclear. I opened a separate issue where I described the problems I see #86. Could you please take a look and share your thoughts?

model/metrics/faas-metrics.yaml Show resolved Hide resolved
docs/faas/faas-metrics.md Outdated Show resolved Hide resolved
@joaopgrassi joaopgrassi force-pushed the feat/faas-metrics-yaml branch from cfe0e20 to 1cf9e98 Compare August 2, 2023 08:29
@joaopgrassi joaopgrassi force-pushed the feat/faas-metrics-yaml branch from 1cf9e98 to fd64349 Compare August 2, 2023 08:31
@joaopgrassi
Copy link
Member Author

joaopgrassi commented Aug 3, 2023

We discussed in slack together with the initial author, @skonto (than you!) and others about the unclear state of the current FaaS metrics. Here's the summary:

We reached the conclusion that the existing metrics are all "incoming", meaning they are to be recorded by the FaaS instance itself. The problem is that some of the metrics can also be reported by whoever is invoking the FaaS. These are:

  • faas.invoke_duration
  • faas.errors
  • faas.invocations
  • faas.timeouts

From @skonto on initial intentions:

the idea afaik was back then to cover scenarios where one function may invoke another (I am coming from the Knative community and we have a serverless framework that targets on premise not that similar to Lambda etc). So when a functions calls another is a client entity anyway.

For the sake of moving forward with this PR, we then agreed to remove the notion of "client-related" metrics and focus only on FaaS metrics. I will create follow up issues to track the rest of the work:

  1. Discuss/Add new attributes to the FaaS metrics faas.name, cloud.provider and cloud.region
  2. Introduce the "client" metrics under a new namespace faas.client.*. These will have the previously confusing attributes faas.invoked_name, faas.invoked_provider and faas.invoked_region
  3. Discuss the necessity of introducing a faas.total_duration metric (which essentially is faas.init_duration + faas.invoke_duration)
  4. Discuss the need of the faas.invocations metric. As @lmolkova mentioned in FaaS metrics - Not clear which attributes should be added to each metric #86 (comment) it can be derived from faas.invoke_duration as it's a Histogram metric.

Please upvote/leave comments if you agree with this or not. I will then do the follow ups once we have an agreement.

CC @jsuereth @AlexanderWert @lmolkova @Oberon00 @tylerbenson

@Oberon00
Copy link
Member

Oberon00 commented Aug 3, 2023

You may also want to consider dropping the faas.client concept altogether (also from tracing). It seems to only make sense on AWS, and maybe would be better incorporated at https://github.com/open-telemetry/semantic-conventions/blob/main/docs/cloud-providers/aws-sdk.md#aws-service-specific-attributes

@joaopgrassi
Copy link
Member Author

For the "TODO" above, I created separated issues:

@reyang reyang merged commit 9d45283 into open-telemetry:main Aug 7, 2023
@joaopgrassi joaopgrassi deleted the feat/faas-metrics-yaml branch August 7, 2023 15:28
bryce-b pushed a commit to bryce-b/semantic-conventions that referenced this pull request Aug 8, 2023
rapphil pushed a commit to rapphil/semantic-conventions that referenced this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move metric definitions to YAML - FaaS Metrics
9 participants