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: Initial Release SQL Helpers #1263

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

arielvalentin
Copy link
Collaborator

@arielvalentin arielvalentin commented Nov 23, 2024

SQL Helpers provides utilities that are intended for use with SQL database related instrumentations.

The initial release adds support for a Shared DB Context Helper, whose goal is to replace the instrumentation specific helpers.

Future releases will target replacing sanitization/obfuscation helpers.

cc: #1194

Sql Helpers provides support for other gems.

This PR is focing on sharing database related attributes from a parent to a child context..
@arielvalentin arielvalentin changed the title feat: Initial Release feat: Initial Release SQL Helpers Nov 23, 2024
@kaylareopelle
Copy link
Contributor

kaylareopelle commented Dec 10, 2024

Thanks for your work to reduce duplicate code!

I'm getting a little stuck thinking through the future plans for the SQL helpers gem. They may not need to block this PR, and this might not be the right forum to talk this through. Please redirect me if there's a better place.

When we created the SQL obfuscation gem, IIRC we intentionally separated the MySQL-specific code into a separate gem, in the spirit of providing the minimum amount of additional code per library. It seemed like we wanted to keep helper libraries small, despite the potential annoyance of having extra libraries.

I don't see this context management code in the PG instrumentation. However, I see it copied in the MySQL2, Trilogy and Redis libraries.

With this new gem, are we shifting our paradigm a bit?

Even though it's a NoSQL library, Redis could still use the same code. However, I don't think it'd benefit from the SQL Obfuscation code.

At a high level, it seems like the context management code could be beneficial for non-SQL libraries as well, the only unique element is the name of the context key. Should the SQL scope for this helper be removed?

** Update ** - one more thing, while reviewing the Faraday PR, I noticed the code in OpenTelemetry::Common::HTTP::ClientContext is essentially the same, with the main unique characteristic being the name of the context key. I'm not an expert on context. Is this a big enough difference to warrant the duplication? Is there an opportunity to refactor and consolidate this too?

@arielvalentin
Copy link
Collaborator Author

Great questions and observations.

If I'm understanding you correctly, you're seeing
an opportunity here for us to eliminate structural duplication between these context helpers since they are structurally all the same and only differ in the a context key.

I suspect that PG does not have a similar helper because Shopify does not use postgresql. They first introduced this helper for the MySQL2 gem but recently switched to Trilogy and needed the same functionality. I suspect copy/paste modify was the quickest way to do that.

I asked them if there would be any concern in consolidating those and there wasn't any objection.

I did not have plans on going the same for PG but I think it's only right to be symmetric and follow up with some updates there.

My goal with this gem is to have shared helpers for all SQL backed Relational Databases so I don't think of sharing this gem with Redis. In the case of Redis, I think we'd want to look at some use cases to see where it may make sense to reuse the same SQL context helper. I'd be worried that SQL DB semantics would end up inadvertently added to Redis spans. But again that is speculative.

We can chat more about this at the next SIG

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.

2 participants