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

Add spec for tracing Azure Functions #716

Merged
merged 14 commits into from
Jan 12, 2023
Merged

Add spec for tracing Azure Functions #716

merged 14 commits into from
Jan 12, 2023

Conversation

z1c0
Copy link
Contributor

@z1c0 z1c0 commented Oct 28, 2022

This is a first specification draft for tracing Azure Functions.
The scope of this PR is generic instrumentation and the HTTP trigger type.
Please also take a lot at the related draft PR regarding Azure Functions metadata: #712

  • Create PR as draft
  • Approval by at least one other agent
  • Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
    • Remove PM from reviewers if impact on product is negligible
    • Remove agents from reviewers if the change is not relevant for them
  • Approved by at least 2 agents + PM (if relevant)
  • Merge after 7 days passed without objections
    To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
  • Create implementation issues through the meta issue template (this will automate issue creation for individual agents)

@apmmachine
Copy link

apmmachine commented Oct 28, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-01-12T18:42:49.934+0000

  • Duration: 3 min 2 sec

Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

Looks nice overall 👍

Some comments below.

@AlexanderWert AlexanderWert linked an issue Nov 21, 2022 that may be closed by this pull request
@z1c0 z1c0 marked this pull request as ready for review November 30, 2022 07:11
@z1c0 z1c0 requested review from a team as code owners November 30, 2022 07:11
z1c0 and others added 3 commits December 22, 2022 11:43
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Added another suggestion for transaction.name for an HTTP-triggered function after some playing with a custom routePrefix in "host.json".

  • If one specifies routePrefix='foo', then routes are /foo/MyFuncName as expected.
  • If one specifies routePrefix='/foo', then routes are //foo/MyFuncName. However GET /foo/MyFuncName works, so I think we should normalize the transaction name to just have a single leading slash.
  • If one specifies routePrefix='' (the empty string), then routes are /MyFuncName.
  • If one specifies routePrefix='foo/' (i.e. with a trailing slash), then routes are /foo//MyFuncName are the Azure Function host code blows up with:
2023-01-03T22:39:04.082 [Error] A host error has occurred during startup operation '513902a7-f0bb-4586-8da2-1ce9c223e8bc'.Microsoft.AspNetCore.Routing.RouteCreationException : An error occurred while creating the route with name 'HttpFn1' and template '/foo//HttpFn1'. ---> System.ArgumentException : The route template separator character '/' cannot appear consecutively. It must be separated by either a parameter or a literal value. (Parameter 'routeTemplate') ---> Microsoft.AspNetCore.Routing.Patterns.RoutePatternException : The route template separator character '/' cannot appear consecutively. It must be separated by either a parameter or a literal value.at Microsoft.AspNetCore.Routing.Patterns.RoutePatternParser.Parse(String pattern)at Microsoft.AspNetCore.Routing.Patterns.RoutePatternFactory.Parse(String pattern)at Microsoft.AspNetCore.Routing.Template.TemplateParser.Parse(String routeTemplate)End of inner exceptionat
...

The last point shows that we don't need to worry about normalizing the number of slashes other than at the start of the URL path.

@trentm
Copy link
Member

trentm commented Jan 12, 2023

For reference: One of the lingering concerns with APM agents running in Azure Functions had been whether the asynchronous intake requests to APM server would have an adverse impact on the billing/cost of those Azure Functions. I did some experiments around this and came to the conclusion that: no, there is no impact on cost from the regular background intake requests. My write-up is here: elastic/apm-agent-nodejs#3071 (comment)

Copy link
Member

@trentm trentm 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 one more suggestion to add the config var name.

Co-authored-by: Trent Mick <trent.mick@elastic.co>
Co-authored-by: Trent Mick <trent.mick@elastic.co>
@z1c0 z1c0 merged commit 43f59d8 into main Jan 12, 2023
@z1c0 z1c0 deleted the azure-functions branch January 12, 2023 18:48
@z1c0 z1c0 mentioned this pull request Jan 13, 2023
7 tasks
trentm added a commit to elastic/apm-agent-nodejs that referenced this pull request Jan 16, 2023
Support for tracing/monitoring Azure Functions.
Supported triggers/bindings: HTTP (spec'd), Timer (not spec'd).

Spec: elastic/apm#716
Closes: #3015
Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
trentm added a commit to elastic/apm-agent-nodejs that referenced this pull request Jan 16, 2023
Support for tracing/monitoring Azure Functions.
Supported triggers/bindings: HTTP (spec'd), Timer (not spec'd).

Spec: elastic/apm#716
Closes: #3015
Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
Support for tracing/monitoring Azure Functions.
Supported triggers/bindings: HTTP (spec'd), Timer (not spec'd).

Spec: elastic/apm#716
Closes: elastic#3015
Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
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.

[META 722] Spec: Azure Functions instrumentation
6 participants