Skip to content

Fix a regression in parsing ignore.resources for traces#2797

Merged
arbll merged 5 commits into
DataDog:masterfrom
bmermet:bmermet/fix-trace-agent-ignore-resources
Dec 18, 2018
Merged

Fix a regression in parsing ignore.resources for traces#2797
arbll merged 5 commits into
DataDog:masterfrom
bmermet:bmermet/fix-trace-agent-ignore-resources

Conversation

@bmermet

@bmermet bmermet commented Dec 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR addresses a regression in the parsing of the ini configuration for the trace agent introduced in #2698.

It used to accept strings normal strings, but now requires them to be escaped with quotes. Meaning that:

[trace.ignore]
resource: config.get_config

used to work but is now broken. And given the ini parser strips quotes, the only way I've found to parse a single resource is to do:

[trace.ignore]
resource: '"/health"'

It currently works for multiple resources with the format:

[trace.ignore]
resource: "/health","/500"

But this is not backward compatible with the previous version that was:

[trace.ignore]
resource: "/health,/500"

Motivation

This PR reverts the original behavior to avoid having a non backward compatible change.

Additional Notes

@bmermet
bmermet requested a review from a team as a code owner December 17, 2018 10:41
@bits-bot

bits-bot commented Dec 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

CLA assistant check
All committers have signed the CLA.

@bmermet
bmermet force-pushed the bmermet/fix-trace-agent-ignore-resources branch from 3d7e4f1 to a6c5ec0 Compare December 17, 2018 15:36
@bmermet
bmermet force-pushed the bmermet/fix-trace-agent-ignore-resources branch from a6c5ec0 to 7106e9f Compare December 17, 2018 17:15
@bmermet
bmermet force-pushed the bmermet/fix-trace-agent-ignore-resources branch from 472b90d to 374d066 Compare December 17, 2018 18:14
arbll
arbll reviewed Dec 17, 2018
View reviewed changes
Comment thread pkg/config/legacy/converter_test.go Outdated
expected []string
}{
{"r1", []string{"r1"}},
{"\"r1\",\"r2,\"", []string{"r1", "r2,"}},

@arbll arbll Dec 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: use back-ticks to be able to write raw strings without need for escaping

arbll
arbll previously approved these changes Dec 17, 2018
View reviewed changes

@arbll arbll left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few nits, apart from them, looks good to me

Comment thread pkg/config/legacy/converter_test.go Outdated
{"r1", []string{"r1"}},
{`"r1","r2,"`, []string{"r1", "r2,"}},
{`"r1"`, []string{"r1"}},
{"r1,r2", []string{"r1", "r2"}},

@arbll arbll Dec 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should probably use them for every config to stay consistent and make reading easier

Comment thread pkg/config/legacy/converter.go Outdated
r.LazyQuotes = true
r.Comma = sep

record, err := r.Read()

@arbll arbll Dec 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could you just return r.Read() ?

@bmermet bmermet Dec 18, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, silly me. I copy pasted this from the trace-agent without giving it a thought.

@arbll arbll added this to the 6.9.0 milestone Dec 18, 2018
@arbll arbll added changelog/no-changelog No changelog entry needed and removed changelog/no-changelog No changelog entry needed labels Dec 18, 2018
arbll
arbll previously approved these changes Dec 18, 2018
View reviewed changes

@arbll arbll left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - For anyone looking at this PR and seeing the critical label: there is no impact on the version 6 of the agent. This code is shared with the datadog-trace-agent and impacts the version 5 of the agent.

@olivielpeau

olivielpeau commented Dec 18, 2018

Copy link
Copy Markdown
Member

Note: this PR has a small impact on Agent 6, since it affects the datadog-agent import command. How does the bug affect users that that have an Agent 5 ini config with resource set under [trace.ignore] and run the datadog-agent import command?

@bmermet

bmermet commented Dec 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The bug indeed breaks the import command for people who use it on a iniconfig affected by the bug. It will exit with this error: https://github.com/DataDog/datadog-agent/blob/master/cmd/agent/common/import.go#L67.

@arbll arbll removed the changelog/no-changelog No changelog entry needed label Dec 18, 2018
@arbll

arbll commented Dec 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Let's add a bugfix release note to this PR then

@codecov-io

codecov-io commented Dec 18, 2018 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2797 into master will increase coverage by <.01%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #2797      +/-   ##
==========================================
+ Coverage   57.13%   57.14%   +<.01%     
==========================================
  Files         385      383       -2     
  Lines       24746    24644     -102     
==========================================
- Hits        14139    14082      -57     
+ Misses       9671     9634      -37     
+ Partials      936      928       -8
Impacted Files Coverage Δ
pkg/config/legacy/converter.go 92.87% <81.81%> (+0.08%) ⬆️
pkg/util/containers/metrics/cgroup_metrics.go 33.17% <0%> (-11.18%) ⬇️
pkg/collector/corechecks/containers/docker.go 0.98% <0%> (-5.04%) ⬇️
pkg/logs/input/listener/tailer.go 86% <0%> (-4%) ⬇️
pkg/config/config.go 82.08% <0%> (-0.81%) ⬇️
pkg/logs/auditor/auditor.go 71.12% <0%> (-0.71%) ⬇️
pkg/autodiscovery/providers/providers.go 100% <0%> (ø) ⬆️
pkg/util/kubernetes/apiserver/controllers.go 0% <0%> (ø) ⬆️
pkg/util/containers/metrics/disk.go
pkg/autodiscovery/providers/kube_services.go
... and 4 more

@arbll
arbll merged commit 3c7f89b into DataDog:master Dec 18, 2018
@bmermet bmermet mentioned this pull request Dec 19, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arbll arbll arbll approved these changes

Assignees

No one assigned

Labels

kind/bug severity/critical team/agent-apm trace-agent

Projects

None yet

Milestone

6.9.0

Development

Successfully merging this pull request may close these issues.

5 participants

@bmermet @bits-bot @olivielpeau @arbll @codecov-io