Skip to content

WIP: Low cardinality span names + callback for WSGI #440

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

Closed

Conversation

Skeen
Copy link
Contributor

@Skeen Skeen commented Feb 21, 2020

This PR fixes #409, and has an ASGI counterpart here: ede28b2

@Skeen Skeen requested a review from a team February 21, 2020 16:57
@Skeen Skeen changed the title Low cardinality span names + callback for WSGI WIP: Low cardinality span names + callback for WSGI Feb 21, 2020
@Skeen
Copy link
Contributor Author

Skeen commented Feb 21, 2020

I see the issues with the flask integration, I'll look into it.

"""

def __init__(self, wsgi):
def __init__(self, wsgi, name_callback=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just use get_default_span_name as default here:

def function(argument):
    return "prefix_0_" + argument


class Middleware:

    def __init__(self, callback=function):
        self.callback = callback

print(Middleware().callback("argument"))
print(Middleware(lambda x: "prefix_1_" + x).callback("argument"))
prefix_0_argument
prefix_1_argument

Copy link
Contributor

Choose a reason for hiding this comment

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

The middleware is initialized only once per process right? Is it worth it adding a dedicated callback to init for this? What if the get_default_span_name() is a method on the middleware class? Users can then just override the class and change the default logic. If we don't want users to go through overriding classes, may be we can provide option constructors in addition? Something like, OpenTelemetryMiddleware.with_span_name_setter(name_setter_func).

I don't think this is a big deal but it feels we'll end up adding more and more args to init to allow customizing some behaviour. Might be worth it prescribing sub-classing instead.

Either that or perhaps the callback could be a generic span callback that gets the whole span and can modify the span in any way, setting the name being one.

@lzchen lzchen closed this in #837 Jun 23, 2020
@Skeen Skeen deleted the ticket/409_low_cardinality_span_names branch June 23, 2020 12:28
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.

extensions should use low cardinality span names by default.
4 participants