Skip to content

Fix logging integrations for DroppedSpan spans#602

Merged
basepi merged 3 commits into
elastic:masterfrom
basepi:fixup
Oct 8, 2019
Merged

Fix logging integrations for DroppedSpan spans#602
basepi merged 3 commits into
elastic:masterfrom
basepi:fixup

Conversation

@basepi

@basepi basepi commented Oct 4, 2019

Copy link
Copy Markdown
Contributor

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

beniwohli commented Oct 7, 2019

Copy link
Copy Markdown
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

basepi commented Oct 7, 2019

Copy link
Copy Markdown
Contributor Author

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

@basepi

basepi commented Oct 7, 2019

Copy link
Copy Markdown
Contributor Author

@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.

beniwohli
beniwohli suggested changes Oct 8, 2019
View reviewed changes

@beniwohli beniwohli left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread elasticapm/traces.py
@basepi basepi merged commit 1f4c363 into elastic:master Oct 8, 2019
@beniwohli beniwohli mentioned this pull request Nov 12, 2019
Closed
This was referenced Oct 7, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@beniwohli beniwohli beniwohli approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@basepi @beniwohli