Enable CI Visibility UI by drodriguezhdez · Pull Request #221 · jenkinsci/datadog-plugin

drodriguezhdez · 2021-06-22T08:23:39Z

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 PR enables the configuration for CI Visibility via UI and environment variables:

DATADOG_JENKINS_PLUGIN_TARGET_TRACE_COLLECTION_PORT
DATADOG_JENKINS_PLUGIN_ENABLE_CI_VISIBILITY
DATADOG_JENKINS_PLUGIN_CI_VISIBILITY_CI_INSTANCE_NAME.

Additionally, the PR adds some changes to the current UI to accommodate the CI Visibility feature in a coherent way for the user.

Enable Log Collection is now visible.
CI Visibility can only be enabled if Datadog Agent mode is used.
Use Datadog Agent instead of DogStatsD in the applicable context.
Use help HTML for textboxes in Datadog Agent mode.

Finally, this PR modifies the README.md document to include the new configuration setup for CI Visibility.

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.

fermayo

Reviewed README

Co-authored-by: Fernando Mayo <[email protected]>

AdrianLC

I think I found some mistake on logging otherwise looks good.
It'd be good to have some tests for the logic in the form.

AdrianLC · 2021-06-23T14:20:36Z

+            if(!enableCiVisibility || DatadogClient.ClientType.DSD.name().equals(this.reportWith)) {
+                this.collectBuildTraces = enableCiVisibility;
+            } else {
+                logger.warning("CI Visibility can only be enabled using Datadog Agent mode.");


It seems this would warn if users set the env var DATADOG_JENKINS_PLUGIN_ENABLE_CI_VISIBILITY to False? Is that a bug?

Hummm, no... The warning is shown when:

enableCiVisibility == true && DatadogClient.ClientType.HTTP.name().equals(this.reportWith)

I'm going to change the condition to be clearer.

AdrianLC · 2021-06-23T14:30:22Z

-                // NOTE: Change this when APM Traces was released as public feature.
-                this.setTraceServiceName(DEFAULT_TRACES_SERVICE_NAME);
+                this.setEnableCiVisibility(ciVisibilityData != null && !ciVisibilityData.isNullObject());
+            } catch (Exception ex) {


would this log a severe error when you raise throw new FormException("CI Visibility can only be enabled using Datadog Agent mode.", "collectBuildTraces")? that seems like a mistake

If we cannot parse the configuration, it's an error and we should log it as an error IMHO. If we cannot parse the configuration is not an expected behaviour

yes but you're also throwing a validation exception in this block and that's not an error in parsing

Oh, you're totally right. Good catch.

Co-authored-by: Adrián López Calvo <[email protected]>

Co-authored-by: Fernando Mayo <[email protected]>

drodriguezhdez · 2021-06-24T07:30:45Z

It'd be good to have some tests for the logic in the form.

Yeah, I agree. However, I think we cannot execute unit tests with this logic, but integration tests. We'd need to take a deeper look at how to do it because this configuration does not have any tests yet (even before CI Visibility).

I've tested these changes in real Jenkins instances, so I think we can go forward, unblock the UI, and then we can invest some time into automatize these integration tests.

Co-authored-by: Fernando Mayo <[email protected]>

FlorianVeaux · 2021-06-25T11:12:31Z

 | Global job tags            | A comma separated list of regex to match a job and a list of tags to apply to that job. Tags can include environment variables that are defined in the master jenkins instance. **Note**: Tags can reference match groups in the regex using the `$` symbol, for example: `(.*?)_job_(*?)_release, owner:$1, release_env:$2, optional:Tag3` | `DATADOG_JENKINS_PLUGIN_GLOBAL_JOB_TAGS`      |
 | Send security audit events | Submits the `Security Events Type` of events and metrics (enabled by default).                                                                                                                                                                | `DATADOG_JENKINS_PLUGIN_EMIT_SECURITY_EVENTS` |
 | Send system events         | Submits the `System Events Type` of events and metrics (enabled by default).                                                                                                                                                                  | `DATADOG_JENKINS_PLUGIN_EMIT_SYSTEM_EVENTS`   |
-| Enable Log Collection      | Collect and Submit build logs (disabled by default).                                                                                                                                                                  | `DATADOG_JENKINS_PLUGIN_COLLECT_BUILD_LOGS`   |


Why are you removing this ?

We're not removing this, but moving to another place in the README. These envvars are related to the Advanced tab, where the "Enable Log Collection" is not there anymore.

Daniel Rodriguez Hernandez added 8 commits June 14, 2021 09:03

Default configuration

a9efbfa

Merge branch 'master' into drodriguezhdez/show_ciapp_UI

a7fc2e2

Edit UI for CI Visibility

e3abfac

Add help

0a5591b

Enable CI Visibility in the Jenkins plugin UI

888209c

Fix typo

4eeade3

Avoid throwing exception if CI Visibility config cannot be processed

f6bf735

Accomodate the methods to the new CI Visibility properties

5396f29

github-actions Bot added the documentation Documentation related changes label Jun 22, 2021

drodriguezhdez added the changelog/Added Added features results into a minor version bump label Jun 22, 2021

drodriguezhdez requested a review from FlorianVeaux June 22, 2021 15:32

drodriguezhdez marked this pull request as ready for review June 22, 2021 15:33

fermayo reviewed Jun 22, 2021

View reviewed changes

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md

Comment thread README.md Outdated

drodriguezhdez and others added 2 commits June 23, 2021 08:52

Apply suggestions from code review

8f277fc

Co-authored-by: Fernando Mayo <[email protected]>

Apply suggestions from code review

37642a6

Co-authored-by: Fernando Mayo <[email protected]>

AdrianLC suggested changes Jun 23, 2021

View reviewed changes

drodriguezhdez and others added 2 commits June 23, 2021 16:40

Apply suggestions from code review

3474585

Co-authored-by: Adrián López Calvo <[email protected]>

Clearer conditions

055f14c

fermayo reviewed Jun 23, 2021

View reviewed changes

Comment thread README.md

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

drodriguezhdez and others added 3 commits June 24, 2021 08:59

Apply suggestions from code review

a647e0a

Co-authored-by: Fernando Mayo <[email protected]>

Apply feedback suggestions

a73cd92

Apply suggestions from code review

47c98bb

Co-authored-by: Fernando Mayo <[email protected]>

AdrianLC previously approved these changes Jun 24, 2021

View reviewed changes

fermayo previously approved these changes Jun 24, 2021

View reviewed changes

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md

drodriguezhdez dismissed stale reviews from fermayo and AdrianLC via b9fd634 June 25, 2021 07:19

Apply suggestions from code review

b9fd634

Co-authored-by: Fernando Mayo <[email protected]>

FlorianVeaux reviewed Jun 25, 2021

View reviewed changes

hithwen previously approved these changes Jun 25, 2021

View reviewed changes

sarina-dd previously approved these changes Jun 25, 2021

View reviewed changes

Include check connectivity CTA button

70dbf6a

drodriguezhdez dismissed stale reviews from sarina-dd and hithwen via 70dbf6a June 28, 2021 13:08

Daniel Rodriguez Hernandez added 2 commits June 28, 2021 16:48

Change 'Connectivity' to 'Connection'

60ee64f

lowercase connectivity str

2d19d53

FlorianVeaux approved these changes Jun 29, 2021

View reviewed changes

drodriguezhdez merged commit d38e759 into master Jun 29, 2021

drodriguezhdez deleted the drodriguezhdez/show_ciapp_UI branch June 29, 2021 07:45

bradq mentioned this pull request Mar 26, 2022

Datadog endpoint for logs should be unbound from metrics/trace host. #280

Open

Uh oh!

Conversation

drodriguezhdez commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Requirements for Contributing to this repository

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)

Uh oh!

fermayo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AdrianLC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drodriguezhdez commented Jun 24, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

drodriguezhdez commented Jun 22, 2021 •

edited

Loading