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 span hooks for web framework integrations #351

Merged
merged 1 commit into from
Nov 5, 2018
Merged

Conversation

rochdev
Copy link
Member

@rochdev rochdev commented Nov 5, 2018

This PR adds span hooks for web framework integrations. The hooks allow users to make modifications to the span just before it's finished.

A few use cases:

  • Changing the resource name to customize how traces are grouped in the UI
  • Adding additional metadata

@rochdev rochdev added enhancement New feature or request integrations labels Nov 5, 2018
@rochdev rochdev added this to the 0.7.0 milestone Nov 5, 2018
@rochdev
Copy link
Member Author

rochdev commented Nov 5, 2018

@brettlangdon The current behavior for spans that are augmented is to keep both hooks. For example, an Express request will start its life as an http.request from the http integration, and then be converted to a express.request in the express integration. In such a case, hooks for both http and express will run before the request span is finished.

Do you think this behavior makes sense or should we simply keep the last hook, in this case the express one?

@brettlangdon
Copy link
Member

For Python we are only going to implement integration specific hooks, e.g. express.request.

I assume for most people that is probably where they will implement hooks, but it seems harmless to support both?

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

lgtm, implementation makes sense 👍

@rochdev
Copy link
Member Author

rochdev commented Nov 5, 2018

I assume for most people that is probably where they will implement hooks, but it seems harmless to support both?

It's definitely an edge case. We could always reevaluate this if it ever causes an issue.

@rochdev rochdev merged commit 2034878 into v0.7.0 Nov 5, 2018
@rochdev rochdev deleted the http-hooks branch November 5, 2018 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants