Make `log_level` centrally configurable #859

axw · 2020-12-10T06:31:01Z

$ELASTIC_APM_LOG_LEVEL now defines the local value, and this can be overridden via central config. If central config is updated and then removed, we'll revert back to the local value like with other config.

If a custom logger is defined, or no log file is specified, the log_level attribute is ignored regardless of whether central config is used. If a custom logger is defined we will log a warning to it about the central config change being ineffective.

Closes #829

Expose the concrete LevelLogger type, so we can add a means of updating the level via central config.

Also expose the default level as a constant, and the level constants.

$ELASTIC_APM_LOG_LEVEL now defines the local value, and this can be overridden via central config. If central config is updated and then removed, we'll revert back to the local value like with other config. If a custom logger is defined, the log_level attribute is ignored regardless of whether central config is used.

apmmachine · 2020-12-10T06:39:15Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #859 updated
Start Time: 2020-12-10T07:42:35.488+0000
Duration: 23 min 28 sec

Test stats 🧪

Test	Results
Failed	0
Passed	7660
Skipped	188
Total	7848

codecov-io · 2020-12-10T07:59:29Z

Codecov Report

Merging #859 (4e2e909) into master (251ea72) will decrease coverage by 0.10%.
The diff coverage is 63.04%.

@@            Coverage Diff             @@
##           master     #859      +/-   ##
==========================================
- Coverage   83.82%   83.72%   -0.11%     
==========================================
  Files         142      142              
  Lines        7888     7913      +25     
==========================================
+ Hits         6612     6625      +13     
- Misses        868      879      +11     
- Partials      408      409       +1

Impacted Files	Coverage Δ
internal/apmcloudutil/provider.go	`80.00% <ø> (ø)`
utils.go	`76.41% <0.00%> (ø)`
config.go	`71.17% <38.46%> (-2.04%)`	⬇️
internal/apmlog/logger.go	`80.82% <70.37%> (-1.00%)`	⬇️
tracer.go	`88.75% <100.00%> (+0.08%)`	⬆️
transport/http.go	`83.33% <0.00%> (-1.37%)`	⬇️
profiling.go	`71.92% <0.00%> (+3.50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 251ea72...4e2e909. Read the comment docs.

simitt · 2020-12-10T08:32:24Z

internal/apmlog/logger.go

 		return "warn"
-	case errorLevel:
+	case ErrorLevel:


CriticalLevel is missing

Nice catch, thanks!

simitt · 2020-12-10T08:57:45Z

tracer.go

@@ -426,6 +426,14 @@ func newTracer(opts TracerOptions) *Tracer {
 	t.setLocalInstrumentationConfig(envSanitizeFieldNames, func(cfg *instrumentationConfigValues) {
 		cfg.sanitizedFieldNames = opts.sanitizedFieldNames
 	})
+	if apmlog.DefaultLogger != nil {
+		defaultLogLevel := apmlog.DefaultLogger.Level()


Hasn't this already potentially changed from the local ENV to the remote config? I don't see how this falls back to the original value in case a remote config value was provided but isn't any longer.

Not 100% sure I follow your question.

apmlog.DefaultLogger.Level() is called when constructing the Tracer, so there's no remote config yet received. We're just capturing the initial (default/environment variable) value.

the function literal passed into t.setLocalInstrumentationConfig below will be invoked immediately (having no effect, since it's just setting the level to the same value), and also when remote config is set and later removed. The latter bit is taken care of by

apm-agent-go/config.go

Lines 407 to 417 in 879c34a

for k := range old {

if _, ok := attrs[k]; ok {

continue

}

updates = append(updates, func(cfg *instrumentationConfig) {

if f, ok := cfg.local[envName(k)]; ok {

f(&cfg.instrumentationConfigValues)

}

})

debugf("central config update: reverted %s to local config", k)

}

I missed defaultLogLevel := apmlog.DefaultLogger.Level()
in https://github.com/elastic/apm-agent-go/pull/859/files#diff-4e84387cbc4fa01c5f9e572c02575e6c8536608159891072d2fbfa69af83a86fR430 - all good then, thanks.

simitt · 2020-12-10T08:58:52Z

config_test.go

@@ -97,9 +100,31 @@ func TestTracerCentralConfigUpdate(t *testing.T) {
 		assert.Len(t, payloads.Transactions[0].Context.Request.Cookies, 1)
 		return payloads.Transactions[0].Context.Request.Cookies[0].Value == "[REDACTED]"
 	})
+	t.Run("log_level", func(t *testing.T) {


I might be missing something - but are there any tests for actually applying the remote log level and also falling back to the local config if a remote config is no longer provided?

testTracerCentralConfigUpdate (which this test calls) does that in

apm-agent-go/config_test.go

Lines 142 to 165 in 879c34a

tracer.SetLogger(apmtest.NewTestLogger(t))

assert.False(t, isRemote(tracer))

timeout := time.After(10 * time.Second)

// We each response payload twice, which causes us to block until

// the first one is fully consumed.

for i := 0; i < 2; i++ {

select {

case responses <- response{etag: "foo", body: serverResponse}:

case <-timeout:

t.Fatal("timed out waiting for config update")

}

}

assert.True(t, isRemote(tracer))

for i := 0; i < 2; i++ {

select {

case responses <- response{etag: "bar", body: "{}"}:

case <-timeout:

t.Fatal("timed out waiting for config update")

}

}

assert.False(t, isRemote(tracer))

.

Each of these sub-tests defines a config key, value to set remotely, and a predicate that indicates whether the remote config has been applied. The code block I pointed to first checks that with the initial tracer config the predicate returns false; then sets the remote config and checks the predicate returns true; then clears out all remote config and again checks that the predicate returns false.

trentm · 2020-12-10T16:21:15Z

internal/apmlog/logger.go

 	case "info":
-		return infoLevel, nil
+		return InfoLevel, nil
 	case "warn":


(This is my first time looking at the go agent, and really reviewing much Go code at all, so feel free to ignore if I'm missing something obvious.)

The spec uses "warning" rather than "warn". Is this function parsing level strings from central config?

@trentm Thanks, nice pick up! You're quite right, I missed that. I'll open another PR to fix it.

elastic/apm-agent-go#859 adds central config support for 'log_level' to the Go agent, so we can now enable it in the UI too. Co-authored-by: Kibana Machine <[email protected]>

elastic/apm-agent-go#859 adds central config support for 'log_level' to the Go agent, so we can now enable it in the UI too. Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

axw added 6 commits December 10, 2020 11:11

Refactor internal/apmlog

b07a745

Expose the concrete LevelLogger type, so we can add a means of updating the level via central config.

internal/apmlog: add method for updating level

3f1699f

internal/apmlog: expose InitDefaultLogger

ee0ccf6

internal/apmlog: expose env var constants

52d67fa

internal/apmlog: expose DefaultLogger.Level

179f2b8

Also expose the default level as a constant, and the level constants.

github-actions bot added the agent-go label Dec 10, 2020

axw mentioned this pull request Dec 10, 2020

[APM] enable 'log_level' for Go elastic/kibana#85511

Merged

2 tasks

axw requested a review from a team December 10, 2020 06:35

axw added 2 commits December 10, 2020 14:42

internal/apmcloudutil: fix test

9636bdb

internal/apmlog: fix lint issues

4e2e909

simitt reviewed Dec 10, 2020

View reviewed changes

simitt approved these changes Dec 10, 2020

View reviewed changes

axw merged commit 491755f into elastic:master Dec 10, 2020

axw deleted the log-level-central-config branch December 10, 2020 10:56

trentm reviewed Dec 10, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `log_level` centrally configurable #859

Make `log_level` centrally configurable #859

axw commented Dec 10, 2020

apmmachine commented Dec 10, 2020 •

edited

Loading

Build stats

Test stats 🧪

codecov-io commented Dec 10, 2020

simitt Dec 10, 2020

axw Dec 10, 2020

simitt Dec 10, 2020

axw Dec 10, 2020

simitt Dec 10, 2020

simitt Dec 10, 2020

axw Dec 10, 2020

trentm Dec 10, 2020

axw Dec 14, 2020

	for k := range old {
	if _, ok := attrs[k]; ok {
	continue
	}
	updates = append(updates, func(cfg *instrumentationConfig) {
	if f, ok := cfg.local[envName(k)]; ok {
	f(&cfg.instrumentationConfigValues)
	}
	})
	debugf("central config update: reverted %s to local config", k)
	}

	tracer.SetLogger(apmtest.NewTestLogger(t))
	assert.False(t, isRemote(tracer))

	timeout := time.After(10 * time.Second)

	// We each response payload twice, which causes us to block until
	// the first one is fully consumed.
	for i := 0; i < 2; i++ {
	select {
	case responses <- response{etag: "foo", body: serverResponse}:
	case <-timeout:
	t.Fatal("timed out waiting for config update")
	}
	}
	assert.True(t, isRemote(tracer))

	for i := 0; i < 2; i++ {
	select {
	case responses <- response{etag: "bar", body: "{}"}:
	case <-timeout:
	t.Fatal("timed out waiting for config update")
	}
	}
	assert.False(t, isRemote(tracer))

Make log_level centrally configurable #859

Make log_level centrally configurable #859

Conversation

axw commented Dec 10, 2020

apmmachine commented Dec 10, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

codecov-io commented Dec 10, 2020

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `log_level` centrally configurable #859

Make `log_level` centrally configurable #859

apmmachine commented Dec 10, 2020 •

edited

Loading