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

Use enum for MessageEvent Type, Link Type and SpanKind#334

Merged
mayurkale22 merged 2 commits intocensus-instrumentation:masterfrom
mayurkale22:enum_for_link_message_event_spankind
Feb 8, 2019
Merged

Use enum for MessageEvent Type, Link Type and SpanKind#334
mayurkale22 merged 2 commits intocensus-instrumentation:masterfrom
mayurkale22:enum_for_link_message_event_spankind

Conversation

@mayurkale22
Copy link
Copy Markdown
Member

Fixes #333

/** An event describing a message sent/received between Spans. */
export enum MessageEventType {
/** Unknown event type. */
UNSPECIFIED = 0,
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.

I do like the numeric enums here so on balance I recommend keeping them this way (nice correspondence with the protos too). But on the other hand, use of a string enum could enable the breaking change to be less significant since the strings 'SENT' and 'RECEIVED' would still be valid. Just mentioning it because it's in my mind, but recommend keeping it this way!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I compared with other lang. implementation, we use numeric enums for these (Kind, Link and MessageEvent) types. I am trying to get most of the breaking change in this and next release before going to Beta.

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.

Makes sense. The sooner the breaking changes come the less the pain in the future.

@mayurkale22 mayurkale22 force-pushed the enum_for_link_message_event_spankind branch from c481b6b to bc2791f Compare February 8, 2019 01:47
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #334 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   94.93%   94.98%   +0.04%     
==========================================
  Files         120      120              
  Lines        8315     8332      +17     
  Branches      739      741       +2     
==========================================
+ Hits         7894     7914      +20     
+ Misses        421      418       -3
Impacted Files Coverage Δ
src/instana.ts 75.86% <0%> (-0.41%) ⬇️
test/test-grpc.ts 99.58% <0%> (ø) ⬆️
test/test-root-span.ts 100% <0%> (ø) ⬆️
test/test-exporter-buffer.ts 100% <0%> (ø) ⬆️
test/test-ocagent.ts 91.74% <0%> (ø) ⬆️
test/test-console-exporter.ts 100% <0%> (ø) ⬆️
test/test-tracer.ts 100% <0%> (ø) ⬆️
test/test-instana.ts 100% <0%> (ø) ⬆️
src/trace/model/span-base.ts 98.91% <0%> (ø) ⬆️
test/test-span.ts 100% <0%> (ø) ⬆️
... and 7 more

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 a692467...bc2791f. Read the comment docs.

@mayurkale22 mayurkale22 merged commit 95be16e into census-instrumentation:master Feb 8, 2019
@mayurkale22 mayurkale22 deleted the enum_for_link_message_event_spankind branch February 8, 2019 19:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants