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

Short-circuit StageData creation in trace generation #430

Conversation

msbit01
Copy link
Contributor

@msbit01 msbit01 commented Jun 3, 2024

Requirements for Contributing to this repository

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must only fix one issue at the time.
  • The pull request must update the test suite to demonstrate the changed functionality.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see CONTRIBUTING.

What does this PR do?

This is a fix for #429. This change will cause trace generation to short circuit before creating an unbounded number of StageData objects.

Description of the Change

Prevent creation of an unbounded number of StageData objects by limiting the number of objects created to be at most the number of characters allowed in the json object created from said objects. This is a conservative approach that could be refined but is simple and good enough in that it avoids unbounded object creation.

Alternate Designs

I considered computing the json and keeping track of the running length of the json object that would be created and checking to see if its length would exceed the limit. That would be a more precise approach than simply counting the number of stages but as my goal was just to bound the number of stages and not necessarily have the bound be perfect, I preferred this approach for its simplicity.

Possible Drawbacks

Verification Process

I confirmed that a test version of the plugin didn't result in the generation of millions of StageData objects as I had seen in previous tests.

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@msbit01 msbit01 force-pushed the 429_short-circuit-stage-data-creation branch from 6d71158 to cbf095a Compare June 3, 2024 18:46
@msbit01 msbit01 changed the title 429: Short-circuit StageData creation in trace generation Short-circuit StageData creation in trace generation Jun 3, 2024
Copy link
Collaborator

@nikita-tkachenko-datadog nikita-tkachenko-datadog left a comment

Choose a reason for hiding this comment

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

Hi @msbit01, many thanks for submitting this fix!
It looks good enough to me, I agree that limiting the number of stages is a simple approach that is efficient enough, given that the second check that verifies the final JSON length is still there.

@nikita-tkachenko-datadog nikita-tkachenko-datadog added the changelog/Fixed Fixed features results into a bug fix version bump label Jun 3, 2024
@tsmsbit
Copy link

tsmsbit commented Jun 3, 2024

@nikita-tkachenko-datadog thanks for the rapid feedback! When do you think it would get released?

@nikita-tkachenko-datadog
Copy link
Collaborator

@nikita-tkachenko-datadog thanks for the rapid feedback! When do you think it would get released?

I will try to do a release tomorrow

@nikita-tkachenko-datadog nikita-tkachenko-datadog merged commit d6122e0 into jenkinsci:master Jun 3, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Fixed Fixed features results into a bug fix version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants