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

Parsing sql for operation name #1013

Merged

Conversation

Falmarri
Copy link
Contributor

@Falmarri Falmarri commented May 4, 2021

No description provided.

@SimunKaracic
Copy link
Contributor

Thanks for the PR 🎉
The original discussion:
https://gitter.im/kamon-io/Kamon?at=609069f84d0065337e4f8116

We'll take a look at it and drop feedback here, but we're swamped for the next couple of days so you'll have to be a bit patient!

@SimunKaracic
Copy link
Contributor

So, this looks great, and we'd like to merge it.
However, we still need to figure out if it has a large performance penalty.
For now, what we should do is hide this feature behind a flag, merge it, and show people how to disable/enable it.

What do you say, are you up for it? :D

@Falmarri
Copy link
Contributor Author

Yeah I'll mess with it a bit as I get some time. The one thing I wanted to do was to add a cache of statements, but wasn't sure how to add guava and have it be shaded (i don't think I'm shading the sql parser either). But maybe that can be done after we see the performance impact

@SimunKaracic
Copy link
Contributor

I think guava might be a bit overkill, a simple TrieMap might be better.
But yeah, let's see the performance impact first.
Add a flag to the reference.conf, fixup the merge conflicts, and we'll take it from there

@Falmarri Falmarri force-pushed the feature/sql-operation-name-parsing branch 6 times, most recently from 0a04586 to 95cc7da Compare May 24, 2021 23:01
@Falmarri Falmarri force-pushed the feature/sql-operation-name-parsing branch from 95cc7da to d6ed380 Compare May 24, 2021 23:46
Copy link
Contributor

@SimunKaracic SimunKaracic left a comment

Choose a reason for hiding this comment

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

Looks good for now, I'll test it later today/tomorrow and come back with some more feedback (if necessary)

@SimunKaracic
Copy link
Contributor

This is about 90% done!
@Falmarri do you have time to finish it?
If you don't I'd be glad to take this over from you

@Falmarri
Copy link
Contributor Author

Falmarri commented Jul 9, 2021

@SimunKaracic Feel free to finish it. I'm not sure when I'll have time but it probably won't be for a week or so.

@SimunKaracic SimunKaracic force-pushed the feature/sql-operation-name-parsing branch from 56db2f5 to 1dc3243 Compare July 12, 2021 11:37
@SimunKaracic SimunKaracic changed the title WIP parsing sql for operation name Parsing sql for operation name Jul 19, 2021
@SimunKaracic SimunKaracic merged commit ba3eb83 into kamon-io:master Jul 19, 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.

2 participants