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

fix parent_id formatting inconsistency #1833

Merged
merged 4 commits into from
May 10, 2021

Conversation

codeboten
Copy link
Contributor

Description

Fixes #1832, funny enough the documentation was showing the parent_id with a leading 0x which means it's likely this was working at some point.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been updated
  • Documentation has been updated

@codeboten codeboten requested review from a team, owais and ocelotl and removed request for a team May 10, 2021 15:26
@@ -1233,7 +1236,7 @@ def test_to_json(self):
"trace_state": "[]"
},
"kind": "SpanKind.INTERNAL",
"parent_id": null,
"parent_id": "0x00000000deadbef0",
Copy link
Contributor

Choose a reason for hiding this comment

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

😂

elif isinstance(self.parent, SpanContext):
parent_id = trace_api.format_span_id(self.parent.span_id)
parent_id = "0x{}".format(
trace_api.format_span_id(self.parent.span_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this. Doesn't format_span_id already format as hex and wouldn't this add 0x twice to the string?

Copy link
Contributor Author

@codeboten codeboten May 10, 2021

Choose a reason for hiding this comment

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

it changes the values to hex, but doesnt prepend the string w/ 0x, see the _format_context method at line 526

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a gap in my understanding but wouldn't that be inconsistent with trace id and span id values then? Seems weird that we'd render parent_id differently from span/trace ID. Are we forced to do this because we documented it this way? Sounds like a bug in the documentation then :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

format context prepends 0x to the trace_id and span_id

    @staticmethod
    @staticmethod
    def _format_context(context):
    def _format_context(context):
        x_ctx = OrderedDict()
        x_ctx = OrderedDict()
        x_ctx["trace_id"] = "0x{}".format(
        x_ctx["trace_id"] = "0x{}".format(
            trace_api.format_trace_id(context.trace_id)
            trace_api.format_trace_id(context.trace_id)
        )
        )
        x_ctx["span_id"] = "0x{}".format(
        x_ctx["span_id"] = "0x{}".format(
            trace_api.format_span_id(context.span_id)
            trace_api.format_span_id(context.span_id)
        )
        )
        x_ctx["trace_state"] = repr(context.trace_state)
        x_ctx["trace_state"] = repr(context.trace_state)
        return x_ctx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change makes the formatting consistent in parent_id as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Makes sense.

@codeboten codeboten merged commit 6f30160 into open-telemetry:main May 10, 2021
@codeboten codeboten deleted the codeboten/fix-1832 branch May 10, 2021 19:12
owais pushed a commit to owais/opentelemetry-python that referenced this pull request May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inconsistency printing to json from ConsoleSpanExporter
3 participants