Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Add message sizes to addMessageEvent function#344

Merged
mayurkale22 merged 3 commits intomasterfrom
add-message-event-fn
Feb 14, 2019
Merged

Add message sizes to addMessageEvent function#344
mayurkale22 merged 3 commits intomasterfrom
add-message-event-fn

Conversation

@draffensperger
Copy link
Copy Markdown
Contributor

@draffensperger draffensperger commented Feb 13, 2019

Message size parameters were added in #323 and this now adds them to the addMessageEvent function on span as optional parameters.

This also adds a comment to the id field of the MessageEvent interface to explain that it is a hexadecimal string that should increment as new messages are sent.

cc/ @odeke-em

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 13, 2019

Codecov Report

Merging #344 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
- Coverage   95.17%   95.14%   -0.03%     
==========================================
  Files         122      122              
  Lines        8472     8474       +2     
  Branches      750      750              
==========================================
  Hits         8063     8063              
- Misses        409      411       +2
Impacted Files Coverage Δ
src/stackdriver-monitoring.ts 81.05% <0%> (-2.11%) ⬇️
test/test-span.ts 100% <0%> (ø) ⬆️
src/trace/model/types.ts 100% <0%> (ø) ⬆️
src/trace/model/span-base.ts 98.92% <0%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82b26d2...1ac7cae. Read the comment docs.

Copy link
Copy Markdown
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this. We need to export these fields in the Stackdriver exporter (https://github.com/census-instrumentation/opencensus-node/blob/master/packages/opencensus-exporter-stackdriver/src/stackdriver-cloudtrace-utils.ts#L84), might be in new PR.

Copy link
Copy Markdown
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Cool, thank you @draffensperger for the follow-up and for adding them here too!

@draffensperger
Copy link
Copy Markdown
Contributor Author

Yes, sure, I can add the exporter logic in a new PR. Probably need it for Stackdriver and agent.

@mayurkale22 mayurkale22 merged commit a687f7f into master Feb 14, 2019
@mayurkale22 mayurkale22 deleted the add-message-event-fn branch February 14, 2019 02:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants