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

Quantize sidekiq args #1972

Merged
merged 15 commits into from
May 6, 2022
Merged

Quantize sidekiq args #1972

merged 15 commits into from
May 6, 2022

Conversation

dudo
Copy link
Contributor

@dudo dudo commented Apr 8, 2022

Allows us to strip out PII for incoming job args. This could probably replace tag_args, or maybe even put the hash in tag_args?? Not sure if that would cause confusion.

@dudo dudo requested a review from a team April 8, 2022 21:52
Brett C. Dudo added 2 commits April 8, 2022 15:22
@dudo
Copy link
Contributor Author

dudo commented Apr 8, 2022

Couple thoughts...

  • secret arg does seem to be obfuscated? Though that might be a red herring.
  • can't seem to get the incantation correct to show the rest of the args.
    • expected: "["random_id"]"
      got: "["?"]"

@delner
Copy link
Contributor

delner commented Apr 13, 2022

Adding quantize and using the existing Hash quantization seems like the way to go.

Not sure about the show: :all obfuscating other args. I would write a unit test case for the Hash quantization module to test the options your passing have the expected result, and if it doesn't pass, it should help you debug why.

@dudo
Copy link
Contributor Author

dudo commented Apr 13, 2022

@delner will submit another PR regarding Hash quantization... the current code early outs with :all. This PR looks good to go as long as we're comfortable deprecating tag_args.

@delner
Copy link
Contributor

delner commented Apr 27, 2022

Not sure about the breaking change in removing tag_args here. Does quantize provide an equivalent?

If so, we must....

  • Have tag_args set the equivalent quantize settings (so users with tag_args are unaffected)
  • Raise a warning when tag_args is accessed and suggest quantize should be used instead.
  • Add appropriate documentation to GettingStarted.md
  • Mark this to be deleted as a breaking change in 2.0.

If we can't do these, we shouldn't merge this as a breaking change.

@marcotc
Copy link
Member

marcotc commented Apr 27, 2022

As a follow up to @delner's point:

@dudo if this is addressed, as well as restoring the per-worker configuration, I think this will be good to merge!

Thank you again for your work!

@marcotc marcotc self-assigned this May 6, 2022
@marcotc marcotc added integrations Involves tracing integrations community Was opened by a community member labels May 6, 2022
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Thank you so much, @dudo!

@marcotc marcotc merged commit c5ea1ac into DataDog:master May 6, 2022
@github-actions github-actions bot added this to the 1.1.0 milestone May 6, 2022
@marcotc
Copy link
Member

marcotc commented May 25, 2022

👋 @dudo, this change has been released in v1.1.0.
Thank you again! 🙇

@agrobbin
Copy link
Contributor

agrobbin commented Jul 10, 2022

@marcotc @dudo what is the expected behavior of these quantized arguments when using Sidekiq with a framework like Active Job, which uses a wrapper worker class to function? job['args'] will look something like this:

[
  {
    "job_class"=>"FooJob",
    "job_id"=>"18a1bfcc-8597-4715-9892-6b658e44eb07",
    "provider_job_id"=>nil,
    "queue_name"=>"default",
    "priority"=>nil,
    "arguments"=>[...],
    "executions"=>0,
    "exception_executions"=>{},
    "locale"=>"en",
    "timezone"=>"UTC",
    "enqueued_at"=>"2022-07-10T03:00:35Z"
  }
]

If I'm reading this code correctly, all of those "args" will be quantized and sent as job span tags, which seems unexpected.

For reference, Sidekiq itself does its best to parse arguments from known wrappers to display in the web UI. That said, it doesn't hide the argument serialization details which might be useful in this situation (i.e. _aj_symbol_keys, _aj_serialized, etc.).

@marcotc
Copy link
Member

marcotc commented Sep 6, 2022

@agrobbin, you mention a very pertinent case.
If the arguments are wrapped in a known format, I believe we should handle the most common ones, like you linked in the official Sidekiq UI.

I'll open an issue to track this improvement.

@marcotc
Copy link
Member

marcotc commented Sep 6, 2022

Captured here #2255.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants