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

Conversation

@lzchen
Copy link
Contributor

@lzchen lzchen commented Dec 26, 2019

Addresses #21, and #22

@lmolkova

@lzchen lzchen requested a review from hectorhdzg as a code owner December 26, 2019 22:17
span.context.span_id
),
resultCode="0", # TODO
resultCode="0",

Choose a reason for hiding this comment

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

can we put canonical code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to use the "StatusCanonicalCode"?

Choose a reason for hiding this comment

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

yes

span.context.span_id
),
duration=utils.ns_to_duration(span.end_time - span.start_time),
responseCode="0",

Choose a reason for hiding this comment

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

can we put canonical code here?

),
duration=utils.ns_to_duration(span.end_time - span.start_time),
responseCode="0",
responseCode=str(StatusCanonicalCode.OK.value),

Choose a reason for hiding this comment

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

I mean we have a real status of the span, can we use that one?

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 was trying to keep the old behaviour, but since there were a bunch of TODOs it's safe to assume that we want to change this. I will change it to use the status of the span.

@lzchen lzchen merged commit 301ebb4 into microsoft:master Dec 31, 2019
@lzchen lzchen deleted the id-format branch December 31, 2019 19:27
@lzchen lzchen mentioned this pull request Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants