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.

Sign up for GitHub

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

Jump to bottom

Automatically add the pid of the current process to root spans #147

Closed
wants to merge 3 commits into from
Closed

Automatically add the pid of the current process to root spans #147

wants to merge 3 commits into from

Conversation

bmermet
Copy link
Contributor

@bmermet bmermet commented Jul 6, 2017

No description provided.

@bmermet bmermet requested review from ufoot and palazzem July 6, 2017 14:08
palazzem
palazzem reviewed Jul 6, 2017
View reviewed changes
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

as soon as @ufoot merge this PR, we can simply rebase your PR and merge it! Thank you!

@palazzem palazzem added the core Involves Datadog core libraries label Jul 6, 2017
ufoot
ufoot approved these changes Jul 6, 2017
View reviewed changes
test/tracer_test.rb
assert_equal('test', span.get_tag('env'))
assert_equal('cool', span.get_tag('temp'))
assert_equal('value1', span.get_tag('tag1'))
assert_equal('value2', span.get_tag('tag2'))
Copy link
Member

Choose a reason for hiding this comment

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

mmm, here, to be truely conform to the previous test, we should also assert_equal(4, span.meta.length) to make sure no extra tag creep in. But this is a nitpick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it out on purpose. I have mixed feelings about it. On one hand it's nice to check more tags haven't been added, on the other hand it means that adding automatic tags like the pid here means having to update unrelated tests. If you think that the pros outweigh the cons, I'll add that check.

@ufoot ufoot modified the milestones: 0.7.3, 0.8.0 Jul 6, 2017
@ufoot ufoot changed the base branch from christian/context to master July 20, 2017 10:43
@ufoot
Copy link
Member

ufoot commented Jul 20, 2017

Merged into master.

@ufoot ufoot closed this Jul 20, 2017
@palazzem palazzem deleted the bmermet/add-pid branch October 6, 2017 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@palazzem palazzem palazzem left review comments

@ufoot ufoot ufoot approved these changes

Assignees
No one assigned
Labels
core Involves Datadog core libraries
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bmermet @ufoot @palazzem