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

feat(transaction): span_count.total #553

Merged
merged 2 commits into from
Sep 10, 2018
Merged

feat(transaction): span_count.total #553

merged 2 commits into from
Sep 10, 2018

Conversation

Qard
Copy link
Contributor

@Qard Qard commented Sep 4, 2018

Fixes #552
Depends on #557

Checklist

  • Implement code
  • Add tests

@Qard
Copy link
Contributor Author

Qard commented Sep 4, 2018

One as-yet unanswered question is if span_count should be null on unsampled transactions.

@watson
Copy link
Contributor

watson commented Sep 5, 2018

The tests fail because of #554

I don't like span_count === null when the transaction is upsampled to be honest. I know about the performance implications, but it also just feels "unclean" to send up these JSON objects containing null objects. But I'm not sure if it's just vanity or if there's something deeper 🤔

@Qard
Copy link
Contributor Author

Qard commented Sep 5, 2018

By that, do you mean you'd prefer the span_count is always present, or that it should be undefined?

@watson
Copy link
Contributor

watson commented Sep 5, 2018

I mean that I prefer the JSON document not to include it for un-sampled transactions. There's a lot of optional fields in our payload that I feel would be weird if they were reported as null. But the APM Server will just ignore them (I assume), so I guess there's no technical problem in keeping it as null.

@Qard
Copy link
Contributor Author

Qard commented Sep 5, 2018

I think explicitly defining them as undefined should avoid the hidden-class problem the same as defining as null does. We could try that.

@Qard Qard force-pushed the span-count branch 2 times, most recently from a1abff9 to 73053f1 Compare September 5, 2018 19:37
@Qard
Copy link
Contributor Author

Qard commented Sep 5, 2018

Rebased with #557 to get tests passing.

Copy link
Contributor

@watson watson left a comment

Choose a reason for hiding this comment

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

Once the "parent" PR have been merged and this one rebased, I'm good to merge

@Qard
Copy link
Contributor Author

Qard commented Sep 10, 2018

Updated to reflect the current state of the decision in elastic/apm-server#1241.

@Qard
Copy link
Contributor Author

Qard commented Sep 10, 2018

I've also changed the dropped.total to dropped, as suggested in elastic/apm-server#1241 (comment).

@watson
Copy link
Contributor

watson commented Sep 10, 2018

Should this PR be rebased or squashed and merged?

@Qard
Copy link
Contributor Author

Qard commented Sep 10, 2018

Could squash and merge, if you edit the commit title/message to describe all the changes. Rebase is fine too though.

@watson watson merged commit fc03e9a into elastic:api-v2 Sep 10, 2018
watson pushed a commit to watson/apm-agent-nodejs that referenced this pull request Oct 27, 2018
watson pushed a commit to watson/apm-agent-nodejs that referenced this pull request Nov 6, 2018
watson pushed a commit to watson/apm-agent-nodejs that referenced this pull request Nov 13, 2018
watson pushed a commit to watson/apm-agent-nodejs that referenced this pull request Nov 13, 2018
watson pushed a commit to watson/apm-agent-nodejs that referenced this pull request Nov 13, 2018
@Qard Qard deleted the span-count branch October 4, 2019 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants