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

feat(cassandra4): more attributes #1314

Merged

Conversation

FrankSpitulski
Copy link
Contributor

fixes #1298

@trask
Copy link
Member

trask commented Oct 3, 2020

hey @FrankSpitulski!

we had some discussion recently in #602 and open-telemetry/opentelemetry-specification#968, and want to spec even instrumentation-specific attributes

can you submit a PR to the specification repo first? this is probably where I would start: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md#call-level-attributes-for-specific-technologies

@iNikem
Copy link
Contributor

iNikem commented Oct 20, 2020

@FrankSpitulski Are you interested in open a PR in spec repo adding those semantic attributes?

@FrankSpitulski
Copy link
Contributor Author

Sure, they tagged the issue I made release:after-ga though so I didn't think it was going to go in.

@trask
Copy link
Member

trask commented Oct 21, 2020

Sure, they tagged the issue I made release:after-ga though so I didn't think it was going to go in.

hey @FrankSpitulski, i've asked for clarification open-telemetry/opentelemetry-specification#1058 (comment)

@FrankSpitulski FrankSpitulski force-pushed the feat/cassandra-4/more-attributes branch 4 times, most recently from 7280c2a to 3465108 Compare December 28, 2020 21:03
@FrankSpitulski
Copy link
Contributor Author

@trask should be good now

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks - small nits

@FrankSpitulski FrankSpitulski force-pushed the feat/cassandra-4/more-attributes branch 2 times, most recently from e50164f to 2e634fd Compare January 8, 2021 00:01
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

@FrankSpitulski
Copy link
Contributor Author

@trask @iNikem pinging for review please

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

Thanks for getting this into the specs!!

Base automatically changed from master to main January 26, 2021 05:50
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

@FrankSpitulski thanks for your patience with us, and great work again driving this through the specification process!

@iNikem
Copy link
Contributor

iNikem commented Feb 1, 2021

@FrankSpitulski can you please rebase this PR? Then it can finally be merged. Sorry for such slow progress :(

@FrankSpitulski
Copy link
Contributor Author

@iNikem It's more than a rebase after the changes in #2113. I'll rewrite it to fit as soon as I can.

@FrankSpitulski
Copy link
Contributor Author

FrankSpitulski commented Feb 2, 2021

@iNikem I have rebased and updated the code to use the new SqlStatementSanitizer. As @mateuszrzeszutek mentioned it can no longer handle CREATE statements. It also no longer splits out the table name from the keyspace when the statement has the form <keyspace>.<table>, which I believe should be considered a bug in the SqlStatementSanitizer.

@anuraaga
Copy link
Contributor

anuraaga commented Feb 2, 2021

Let's go ahead and get this in - @mateuszrzeszutek do you mind following up with the SQL issues?

@anuraaga anuraaga merged commit 9ded718 into open-telemetry:main Feb 2, 2021
@anuraaga
Copy link
Contributor

anuraaga commented Feb 2, 2021

@FrankSpitulski Thanks a lot for this contribution and bearing with the long review process! 🙏

@mateuszrzeszutek
Copy link
Member

it can no longer handle CREATE statements

I'll create an issue for that 👍

It also no longer splits out the table name from the keyspace when the statement has the form <keyspace>.<table>, which I believe should be considered a bug in the SqlStatementSanitizer.

Actually, this is intentional: spec says that db.sql.table is

the name of the primary table that the operation is acting upon, including the schema name (if applicable).

so whatever sanitizer extracts as "table" may contain the schema name.
I assume that in Cassandra's case keyspace is equivalent to schema name. I think that the best course of action would be to just split the table string in Cassandra instrumentation.

@FrankSpitulski FrankSpitulski deleted the feat/cassandra-4/more-attributes branch February 2, 2021 16:38
@trask
Copy link
Member

trask commented Feb 4, 2021

🎉 thank you @FrankSpitulski

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.

Additional tags for cassandra 4
5 participants