Add log collection by gzussa · Pull Request #44 · jenkinsci/datadog-plugin

gzussa · 2020-02-13T13:43:29Z

What does this PR do?

Adding log collection and better exception logging when the plugin is misconfigured.

Description of the Change

Created a package to hold the log logic. It contains 3 classes:

DatadogConsoleLogFilter that extends ConsoleLogFilter. All it does is decorate the log outputStream with a custom DatadogOutputStream if needed.
DatadogOutputStream extends LineTransformationOutputStream and mainly clean log lines from Jenkins Notes. It then uses a DatadogWriter class to submit logs.
DatadogWriter gets contextual information to prepare the log message to be sent to Datadog.
- As a side effect, i had to refactor the BuildData in order to be able to retrieve a maximum of tags/contextual info even when a listener is not provided to the constructor. In turn, i had to make changes to some methods in DatadogUtilities in order to be more permissive.

Added new configs:

logIntakeUrl when submitting log using the Datadog API directly.
logCollectionPort when submitting log using the Datadog Agent.
collectBuildLogs to turn the collection on and off (default is off).

Env variable loading is not overriding the saved config anymore, hence simplifying the loading logic a little.

I also made changes regarding exception loggings. Before, if the plugin was misconfigured it would print out stack trace exceptions in stdout. It would do so repetitively because of all Publisher classes.

We no longer throw exceptions when calling getInstance on a client implementation.
Even if params are not valid (null or empty), we return null. As a consequence, I had to add null checks everywhere in order to return from the calling method. Now user will see clean severe level error message in their logs.
I also added connection broken attributes (defaultIntakeConnectionBroken and logIntakeConnectionBroken) in order to prevent making bad http calls to Datadog all the time and especially when we know the connection is broken after validation.
As a consequence, i had to refactor the validate http client method is order to be static and independent from the current client instance singleton.

Possible Drawbacks

When enable, log submission may have some perf implications but i haven't done any load testing at this point and it is kinda inevitable anyway.

Verification Process

Review and manual testing mainly.

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.

zippolyte · 2020-02-21T09:24:38Z

+            if(StringUtils.isNotBlank(portStr)){
+                if(StringUtils.isNumeric(portStr)) {
+                    this.setTargetPort(formData.getInt("targetPort"));
+                }
+            }else{
+                this.setTargetPort(null);
+            }
+            String logCollectionPortStr = formData.getString("targetLogCollectionPort");
+            if(StringUtils.isNotBlank(logCollectionPortStr)){
+                if(StringUtils.isNumeric(logCollectionPortStr)){
+                    this.setTargetLogCollectionPort(formData.getInt("targetLogCollectionPort"));
+                }
+            }else{
+                this.setTargetLogCollectionPort(null);
+            }


isn't it supposed to be validated by validateTargetPort ?

yes and it is happening below.

I guess I don’t understand why we do the type checking here as well

kaykay88 · 2020-04-07T08:35:11Z

@gzussa
In which release is this change added ? I am asking because for 1.1.0 plugin verion, I am still seeing the following error.

2020-04-07 08:07:20.770+0000 [id=39]	SEVERE	o.d.j.p.d.c.DatadogHttpClient#getInstance: Datadog API Key is not set properly

2020-04-07 08:07:23.213+0000 [id=43]	SEVERE	o.d.j.p.d.c.DatadogHttpClient#getInstance: Datadog API Key is not set properly

2020-04-07 08:07:23.245+0000 [id=39]	SEVERE	o.d.j.p.d.c.DatadogHttpClient#getInstance: Datadog API Key is not set properly

2020-04-07 08:07:23.364+0000 [id=41]	SEVERE	o.d.j.p.d.c.DatadogHttpClient#getInstance: Datadog API Key is not set properly

2020-04-07 08:07:23.398+0000 [id=42]	SEVERE	o.d.j.p.d.c.DatadogHttpClient#getInstance: Datadog API Key is not set properly

gzussa · 2020-04-07T08:42:11Z

Hey @kaykay88, it seems like a plugin configuration issue. Another issue has been open #55. We will follow up there.

jbergknoff-rival · 2020-05-28T19:17:05Z

+            DatadogUtilities.severe(logger, null, String.format("Datadog/%s/jenkins Java/%s Jenkins/%s",
+                    getDatadogPluginVersion(),
+                    getJavaRuntimeVersion(),
+                    getJenkinsVersion()));


Is this supposed to be here?

Definitely not and still on master, I'll remove it now 👍

Gregory added 2 commits February 12, 2020 15:04

Adding log collection

5efb2c6

Adding log collection

9da35fc

gzussa requested review from bkabrda, jirikuncar, nmuesch and zippolyte as code owners February 13, 2020 13:43

gzussa changed the title ~~Gzu/logs~~ Add log collection Feb 13, 2020

github-actions Bot added the documentation Documentation related changes label Feb 13, 2020

Improve log endpoint validations

aeb4226

timja mentioned this pull request Feb 13, 2020

Don't log an error if API key not set #45

Closed

6 tasks

Gregory added 6 commits February 14, 2020 10:05

incorporate feedback from [#45]

317dfdc

Improve testing doc

819bd8a

Fix log collection using the Datadog Agent

1c4bad3

incorporate feeback

e48b297

incorporate feeback

7322fc0

updating jenkins version

579b94c

zippolyte suggested changes Feb 21, 2020

View reviewed changes

Gregory added 3 commits February 21, 2020 15:45

incorporate feedback 🚀

9d25366

remove comments:

6efbfd1

upgrade dogstatsd client with event source type fix 🪨

ff07a30

zippolyte approved these changes Feb 21, 2020

View reviewed changes

gzussa merged commit 25c2679 into master Feb 21, 2020

gzussa deleted the gzu/logs branch February 21, 2020 15:00

gzussa mentioned this pull request Feb 21, 2020

Latest plugin version causing exception in system log, when no API token configured #42

Closed

gzussa added the changelog/Added Added features results into a minor version bump label Feb 21, 2020

jbergknoff-rival reviewed May 28, 2020

View reviewed changes

FlorianVeaux mentioned this pull request May 29, 2020

Remove debugging log line #65

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add log collection#44

Add log collection#44
gzussa merged 12 commits into
masterfrom
gzu/logs

gzussa commented Feb 13, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

zippolyte Feb 21, 2020

Uh oh!

gzussa Feb 21, 2020

Uh oh!

zippolyte Feb 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaykay88 commented Apr 7, 2020

Uh oh!

gzussa commented Apr 7, 2020

Uh oh!

jbergknoff-rival May 28, 2020

Uh oh!

FlorianVeaux May 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

gzussa commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Description of the Change

Possible Drawbacks

Verification Process

Review checklist (to be filled by reviewers)

Uh oh!

Uh oh!

Uh oh!

zippolyte Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

gzussa Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

zippolyte Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaykay88 commented Apr 7, 2020

Uh oh!

gzussa commented Apr 7, 2020

Uh oh!

jbergknoff-rival May 28, 2020

Choose a reason for hiding this comment

Uh oh!

FlorianVeaux May 29, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gzussa commented Feb 13, 2020 •

edited

Loading