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

Changed the format of traceparent id #941

Merged
merged 10 commits into from
Mar 16, 2022

Conversation

Thiyagu55
Copy link
Contributor

Changed the format of traceparent id

Compressed the format of traceparent id from /*span_id=16659691402252420308,trace_flags=1,trace_id=283190570959089573593920162873354610508,trace_state=''*/ to /*traceparent='00-91be8b5a37d16542524ae5ae0907ec5f-543bfa9675358c75-1'*/

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • tox -e test-instrumentation-dbapi

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated

@Thiyagu55 Thiyagu55 requested a review from a team as a code owner March 3, 2022 07:06
@Thiyagu55 Thiyagu55 marked this pull request as draft March 3, 2022 07:06
@Thiyagu55 Thiyagu55 marked this pull request as ready for review March 3, 2022 07:20
@ocelotl
Copy link
Contributor

ocelotl commented Mar 9, 2022

What issue is being fixed by this PR, @Thiyagu55 ?

@Thiyagu55
Copy link
Contributor Author

What issue is being fixed by this PR, @Thiyagu55 ?

@ocelotl This is not a bug fix issue.
Changed the format of traceparent id in query log

@ocelotl
Copy link
Contributor

ocelotl commented Mar 9, 2022

What issue is being fixed by this PR, @Thiyagu55 ?

@ocelotl This is not a bug fix issue. Changed the format of traceparent id in query log

Ok, I should rephrase. What is the motivation behind this PR? Why do we want to add this change?

Just to be clear, I am by no means rejecting this PR or considering it not necessary, I just want to know the reasons behind it. ✌️

@srikanthccv
Copy link
Member

@ocelotl The current format is not complaint with the commenter specification. This change fixes it.

@lzchen
Copy link
Contributor

lzchen commented Mar 11, 2022

@Thiyagu55
Could you link to the section of the sqlcommenter documentation in which it specifies the format of traceparent?

@Thiyagu55
Copy link
Contributor Author

Thiyagu55 commented Mar 14, 2022

@Thiyagu55
Could you link to the section of the sqlcommenter documentation in which it specifies the format of traceparent?
@lzchen https://open-telemetry.github.io/opentelemetry-sqlcommenter/

@lzchen
Copy link
Contributor

lzchen commented Mar 14, 2022

Nit: Can you use f strings for this

@lzchen
Copy link
Contributor

lzchen commented Mar 14, 2022

@srikanthccv
Do you think _generate_sql_comment would always be used in conjuction with _generate_opentelemetry_traceparent? Is it necessary to separate these?

@srikanthccv
Copy link
Member

@lzchen traceparent header is only part of the comment, there can be more info embedded in the comment such as request route. I think it is fine they are separate.

@lzchen lzchen merged commit abd01fb into open-telemetry:main Mar 16, 2022
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.

None yet

4 participants