Skip to content

Conversation

basepi
Copy link
Contributor

@basepi basepi commented Oct 4, 2019

The logging integrations assumed (erroneously) that all spans would have an ID. This fixes that assumption, so that DroppedSpans don't cause exceptions.

I also fixed up some tests to use the elasticapm_client fixture instead of manually creating a Tracer object.

@basepi basepi requested a review from beniwohli October 4, 2019 20:31
@beniwohli
Copy link
Contributor

@basepi maybe it would make sense to add an id attribute to DroppedSpan that is always None. That would make it less of a cognitive overhead to always have to remember that there are multiple types of spans. WDYT?

@basepi
Copy link
Contributor Author

basepi commented Oct 7, 2019

Seems reasonable to me. I'll make that change today.

@basepi
Copy link
Contributor Author

basepi commented Oct 7, 2019

@beniwohli Ready for review. Note that I do not include None IDs in the structlog integration. I'm a little unsure on that decision -- in most cases None would mean there isn't a transaction/span/trace but in this case we do have a span but it doesn't have an ID...

Probably not a terribly important distinction but I wanted to call it out explicitly in case you think I should include the field in this case.

Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

other than the __slots__ thing, LGTM!

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.

2 participants