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

contrib/bradfitz/gomemcache: trace the processing keys #640

Closed
mingrammer opened this issue Apr 21, 2020 · 6 comments · Fixed by #642
Closed

contrib/bradfitz/gomemcache: trace the processing keys #640

mingrammer opened this issue Apr 21, 2020 · 6 comments · Fixed by #642
Labels
apm:ecosystem contrib/* related feature requests or bugs proposal more in depth change that requires full team approval

Comments

@mingrammer
Copy link
Contributor

Hi.

Currently, the memcached tracer is just creating a span and does not trace anything except env tag. So, dashboard shows only env variable, and I couldn't find any valuable information such as processing keys.

It would be nice to add the keys tag for tracking the used key for each memcached operations. If you agree with this, I can make a PR for this.

@gbbr
Copy link
Contributor

gbbr commented Apr 21, 2020

Hi @mingrammer!

I'm not sure exactly what you mean here. Can you please try to explain in a different way for me? Maybe post a screenshot of what you're seeing and explain what you're not seeing? Specifically the part where you say:

does not trace anything except env tag. So, dashboard shows only env variable

...is unclear to me

@mingrammer
Copy link
Contributor Author

Here are two screenshots, one is for redis, other is for memcached.

Redis in APM

Screen Shot 2020-04-21 at 7 56 09 PM

It shows various information such as executed command (including used keys), current db and so on.

Memcached in APM

Screen Shot 2020-04-21 at 7 56 41 PM

But the memcached span shows only env tag. I want to track keys used in a memcached operation as in the redis span.

@gbbr
Copy link
Contributor

gbbr commented Apr 21, 2020

Thanks for doing this! That makes complete sense now! I think it should be fine to make this change and add more tags as long as it doesn't reveal any sensitive information!

@mingrammer
Copy link
Contributor Author

Thank you. I’ll send a PR for adding some tags to memcached span.

mingrammer added a commit to mingrammer/dd-trace-go that referenced this issue Apr 21, 2020
Currently, the memcached tracer is just creating a span and does not
trace anything except env tag. This commit adds some tags of valuable
information for each memcached operations.

Fixes DataDog#640
@gbbr gbbr closed this as completed in #642 Apr 22, 2020
gbbr pushed a commit that referenced this issue Apr 22, 2020
…#642)

This commit adds more tags to each memcached operation, along with an option to also add assignment values.

Fixes #640
@knusbaum knusbaum reopened this May 18, 2020
@knusbaum
Copy link
Contributor

knusbaum commented May 18, 2020

Hi, @mingrammer. I have reopened this issue since we had to revert #642.

We can take another look at this issue and come up with another solution.

Including all keys and values in particular was the issue. We can talk about your use case more to understand what a good solution might be.

@knusbaum knusbaum added apm:ecosystem contrib/* related feature requests or bugs proposal more in depth change that requires full team approval labels Sep 16, 2020
mingrammer added a commit to mingrammer/dd-trace-go that referenced this issue Dec 22, 2020
…DataDog#642)

This commit adds more tags to each memcached operation, along with an option to also add assignment values.

Fixes DataDog#640
@knusbaum
Copy link
Contributor

Closing this for now.

Please reopen if we need to revisit this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs proposal more in depth change that requires full team approval
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants