Skip to content

make the plugin see data stream last by adjusting decorator order, to avoid logging unmasked data#296

Merged
steveny91 merged 8 commits into
jenkinsci:masterfrom
fengxx:bugfix/maskdata
Aug 18, 2022
Merged

make the plugin see data stream last by adjusting decorator order, to avoid logging unmasked data#296
steveny91 merged 8 commits into
jenkinsci:masterfrom
fengxx:bugfix/maskdata

Conversation

@fengxx

@fengxx fengxx commented Jul 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Requirements for Contributing to this repository

make the plugin see data stream last by adjusting decorator order, to avoid logging unmasked data
see also:

jenkinsci/workflow-api-plugin#166
jenkinsci/splunk-devops-plugin@1a10a53

  • 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?

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

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.

@fengxx

fengxx commented Jul 9, 2022

Copy link
Copy Markdown
Contributor Author

please note CI failure is expected: ‘Jenkinsfile’ has been modified in an untrusted revision, need newer version in Jenkinsfile to run tests

drodriguezhdez
drodriguezhdez previously requested changes Jul 11, 2022
View reviewed changes

@drodriguezhdez drodriguezhdez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ci.node_name and ci.node_labels can be used to filter data.
I block this PR because this change might break customer dashboards, monitors, etc.

Comment thread ...t/java/org/datadog/jenkins/plugins/datadog/publishers/DatadogQueuePipelinePublisherTest.java Outdated
@fengxx

fengxx commented Jul 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

ci.node_name and ci.node_labels can be used to filter data.

the change just reflects the fact default node name had changed, with or without the change, filter is broken on 2.319.1 or later

@drodriguezhdez
drodriguezhdez dismissed their stale review July 12, 2022 08:21

This does not break dashboards/monitors

@fengxx

fengxx commented Jul 12, 2022

Copy link
Copy Markdown
Contributor Author

This does not break dashboards/monitors

Actually I am not a datadog user, I don't know how the dashboards works, but the logic in src/main/java/org/datadog/jenkins/plugins/datadog/traces/DatadogTraceBuildLogic.java breaks tests which expect something else. If the tests are not reflecting use cases, they need to be updated.

jose-manuel-almaza
jose-manuel-almaza requested changes Jul 12, 2022
View reviewed changes

@jose-manuel-almaza jose-manuel-almaza left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is not convenient to put timeouts in the tests in an approximate way.
The tests must always give the same result, regardless of when we run it, machine load or any other factor external to the test.

Comment thread ...t/java/org/datadog/jenkins/plugins/datadog/publishers/DatadogQueuePipelinePublisherTest.java
@fengxx
fengxx force-pushed the bugfix/maskdata branch from 975b0cd to 298172b Compare July 16, 2022 04:18
drodriguezhdez
drodriguezhdez reviewed Jul 18, 2022
View reviewed changes
Comment thread src/test/java/org/datadog/jenkins/plugins/datadog/listeners/DatadogGraphListenerTest.java Outdated
assertNotNull(buildSpanMeta.get(CITags.NODE_NAME));
assertNotNull(buildSpanMeta.get(CITags.NODE_LABELS));
assertNull(buildSpanMeta.get(CITags._DD_HOSTNAME));
String nodeName=buildSpanMeta.get(CITags.NODE_NAME);

@drodriguezhdez drodriguezhdez Jul 18, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this line required?

Comment thread src/test/java/org/datadog/jenkins/plugins/datadog/listeners/DatadogTraceAbstractTest.java
sarah-witt
sarah-witt reviewed Jul 25, 2022
View reviewed changes

@sarah-witt sarah-witt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @fengxx thanks again for doing the extra work to bump the required minimum jenkins version, I left one small comment

Comment thread Jenkinsfile
Comment thread pom.xml
@yzhan289

yzhan289 commented Aug 17, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

image
I was able to verify that passwords are indeed now masked correctly in Datadog. Looks like there's some CI failures, so I just reran the Jenkins CI. If it passes, this LGTM!

@sarah-witt sarah-witt added the changelog/Changed Changed features results into a major version bump label Aug 18, 2022
@steveny91
steveny91 merged commit 9287065 into jenkinsci:master Aug 18, 2022
This was referenced Aug 19, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drodriguezhdez drodriguezhdez drodriguezhdez left review comments
@sarah-witt sarah-witt sarah-witt approved these changes
@jose-manuel-almaza jose-manuel-almaza jose-manuel-almaza left review comments

Assignees

No one assigned

Labels

changelog/Changed Changed features results into a major version bump

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fengxx @yzhan289 @drodriguezhdez @sarah-witt @jose-manuel-almaza @steveny91