New tracer #3836

jalvz · 2020-06-02T13:53:21Z

Motivation/summary

Checklist

I have signed the Contributor License Agreement.
I have updated CHANGELOG.asciidoc

I have considered changes for:

documentation
logging (add log lines, choose appropriate log selector, etc.)
metrics and monitoring (create issue for Kibana team to add metrics to visualizations, e.g. Kibana#44001)
automated tests (add tests for the code changes, all unit tests pass locally)
telemetry
Elasticsearch Service (https://cloud.elastic.co)
Elastic Cloud Enterprise (https://www.elastic.co/products/ece)
Elastic Cloud on Kubernetes (https://www.elastic.co/elastic-cloud-kubernetes)

How to test these changes

Related issues

elastic/beats#18861

beater/tracing.go

apmmachine · 2020-06-03T13:52:51Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #3836 updated]
Start Time: 2020-06-25T06:23:01.171+0000
Duration: 52 min 39 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3211
Skipped	147
Total	3358

Steps errors

Expand to view the steps failures

Name: Compress
- Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage
- Duration: 0 min 0 sec
- Start Time: 2020-06-25T06:37:49.586+0000
- log
Name: Compress
- Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests
- Duration: 0 min 0 sec
- Start Time: 2020-06-25T06:49:11.117+0000
- log
Name: Test Sync
- Description: ./script/jenkins/sync.sh
- Duration: 3 min 38 sec
- Start Time: 2020-06-25T06:33:09.084+0000
- log

jalvz · 2020-06-18T13:54:02Z

Notes:

make update after beats update failed with

  File "script/generate_notice.py", line 354, in <module>
    modules = gather_modules(args.main_package, args.build_tags)
  File "script/generate_notice.py", line 118, in gather_modules
    print("WARNING: Unknown license for {}: {}".format(modpath, os.path.join(root, filename)))
NameError: name 'root' is not defined

python ./script/generate_notice.py seemed to fix, but don't know why there is a huge diff.

TestServerTracingExternal still failing. But i'd say this should be moved to libbeat. Thoughts?

axw · 2020-06-23T03:45:50Z

make update after beats update failed with

File "script/generate_notice.py", line 354, in
modules = gather_modules(args.main_package, args.build_tags)
File "script/generate_notice.py", line 118, in gather_modules
print("WARNING: Unknown license for {}: {}".format(modpath, os.path.join(root, filename)))
NameError: name 'root' is not defined

There's an error in the script; like it says, root is an undefined variable - my mistake. However, that line is only reached when there's an unknown license found... so that needs to be fixed. I fixed the script locally and it's triggered by https://github.com/elastic/elastic-agent-client/blob/master/LICENSE.txt. We'll just need to add detection of the Elastic License. I'll look into this.

python ./script/generate_notice.py seemed to fix, but don't know why there is a huge diff.

If you run the script directly, it's not going to be run the same as when you run make update. See the Makefile:

apm-server/Makefile

Line 130 in 3d62fee

    
           @$(PYTHON) script/generate_notice.py -b "Elastic APM Server" -s "github.com/elastic/beats*" . ./x-pack/apm-server

I suppose the error is only occurring through make update because it includes x-pack packages, which will import elastic-agent-client.

_meta/beat.yml

axw · 2020-06-23T05:12:49Z

beater/beater.go

+	useLegacyTracer := common.MustNewVersion(version.GetDefaultVersion()).LessThan(&common.Version{Major: 8, Minor: 0})
+
+	if !tracer.Active() && useLegacyTracer {
+		tracer, tracerServer, err = initLegacyTracer(b.Info, bt.config, bt.logger)


Instead of holding onto that old code, can we instead dynamically rename apm-server.instrumentation to instrumentation, if the latter doesn't exist in the config?

I think we can do this by defining a ConfigOverride in libbeat/cmd/instance.Settings, whose Check method checks for existing "instrumentation", and if not specified sets it to "apm-server.instrumentation"

Hmm how would that exactly work?

{ Check: func(cfg *common.Config) bool { return cfg.HasField("instrumentation") }, Config: ???

Config is just a config, not a function, so how do I copy apm-server.instrumentation?

I meant make Check do the modification. It would be a bit of a hack, so it should really be replaced with something more fit for purpose. But in the mean time...

Check: func(cfg *common.Config) bool { if !cfg.HasField("instrumentation") { // Check if cfg has "apm-server.instrumentation", and rename it to "instrumentation" } return true }

ah, uh. that's quite a hack, yes..
ok

How about not using Check but changing libbeatConfigOverride to be a function and taking care of the overwrite there?

But how do I pass to a function the original config I need to copy?
Or you mean to change the whole thing in libbeat?

I presume @simitt meant changing libbeat. I think that's the right long-term solution. IMO, ConditionalOverride should be an implementation of an interface like ConfigTransformer:

type ConfigTransformer interface { // TransformConfig transforms the provided config, returning a possibly modified config. TransformConfig(*common.Config) (*common.Config, error) } type ConditionalOverride struct { Check OverrideChecker Config *common.Config } func (o *ConditionalOverride) TransformConfig(in *common.Config) (*common.Config, error) { if !o.Check(in) { return in, nil } return common.MergeConfigs(in, o.Config) }

We could then provide an alternative implementation of that interface which does what we need.

beater/beater.go

axw · 2020-06-23T07:42:04Z

TestServerTracingExternal still failing. But i'd say this should be moved to libbeat. Thoughts?

That sounds good to me.

jalvz · 2020-06-23T13:54:59Z

So tests relying on beater_tests.newInstrumentation fail now, I'm kinda stuck with those.
I think those tests should actually be moved system tests (i mean the ones in Python).
Anyone against?

axw · 2020-06-24T02:34:21Z

So tests relying on beater_tests.newInstrumentation fail now, I'm kinda stuck with those.
I think those tests should actually be moved system tests (i mean the ones in Python).
Anyone against?

I'm OK with that. It feels a bit heavy, but it's not code that needs to change often so I guess unit tests aren't really needed.

BTW, the issue with those tests is that you created a tracer but didn't configure its transport to send to the listener. If you do stick with the unit tests, maybe use libbeat/instrumentation.New rather than duplicating logic.

beater/tracing.go

axw

LGTM, just a couple of minor things.

axw · 2020-06-25T01:17:07Z

cmd/root.go

+	}),
+},
+	{
+		Check: func(cfg *common.Config) bool {


Can you please add a TODO here to update libbeat, adding a more fit for purpose API?

axw · 2020-06-25T01:21:32Z

cmd/root_test.go

+)
+
+func TestOverrideLegacyInstrumentationConfig(t *testing.T) {
+	cfg, err := cfgfile.Load("tests/legacy_instrumentation.yml", libbeatConfigOverrides)


It's conventional to add test-related data files in a directory called "testdata", which the go tool has a special case to ignore when listing packages and such. Can you please rename?

) * Update to elastic/beats@1498938e22f3

* script: add detection of Elastic License * Support new instrumentation config and tracer from Libbeat (#3836) * Update beats framework to f9b5f18382ac Co-authored-by: Andrew Wilkins <[email protected]>

…lastic#3836)" This reverts commit 6ba318f.

axw reviewed Jun 3, 2020

View reviewed changes

beater/tracing.go Outdated Show resolved Hide resolved

Attempt to use libbeat tracer

fb3c5a5

jalvz force-pushed the new-tracer branch from 8a09d5c to fb3c5a5 Compare June 3, 2020 13:24

Flip the logic

3639af2

jalvz added 4 commits June 17, 2020 13:44

Merge remote-tracking branch 'elastic/master' into new-tracer

981ecfe

fix NOTICE??

1b66e0e

Update beats

a1f60f8

Adapt to latest beats implementation

448daac

jalvz marked this pull request as ready for review June 18, 2020 13:54

jalvz requested a review from axw June 18, 2020 13:54

axw mentioned this pull request Jun 23, 2020

Update beats to elastic/beats@0dcb3df #3909

Merged

2 tasks

axw reviewed Jun 23, 2020

View reviewed changes

jalvz added 6 commits June 23, 2020 11:00

Merge remote-tracking branch 'elastic/master' into new-tracer

ab2e5a1

Remove deprecation notice

351a81a

Remove test

df43748

Fix small bug

2944c67

Code review

e468b8f

Code review

d326674

axw reviewed Jun 24, 2020

View reviewed changes

beater/tracing.go Outdated Show resolved Hide resolved

jalvz added 3 commits June 24, 2020 11:23

more cleanup

a716180

Update to elastic/beats@1498938e22f3

053abd2

add tests

43613a4

axw approved these changes Jun 25, 2020

View reviewed changes

last code review

816a7d0

update changelog

8840b2d

jalvz merged commit 6ba318f into elastic:master Jun 25, 2020

jalvz added a commit to jalvz/apm-server that referenced this pull request Jun 30, 2020

Support new instrumentation config and tracer from Libbeat (elastic#3836

096ce6d

) * Update to elastic/beats@1498938e22f3

jalvz mentioned this pull request Jun 30, 2020

[7.x] New tracer (#3836) #3923

Merged

10 tasks

jalvz added a commit to jalvz/apm-server that referenced this pull request Jul 2, 2020

Revert "Support new instrumentation config and tracer from Libbeat (e…

1ba1007

…lastic#3836)" This reverts commit 6ba318f.

jalvz mentioned this pull request Jul 9, 2020

Legacy tracer config not recognized when passed as command line arg #3950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New tracer #3836

New tracer #3836

jalvz commented Jun 2, 2020 •

edited

Loading

apmmachine commented Jun 3, 2020 •

edited

Loading

Build stats

Test stats 🧪

jalvz commented Jun 18, 2020

axw commented Jun 23, 2020

axw Jun 23, 2020

jalvz Jun 23, 2020

axw Jun 23, 2020

jalvz Jun 23, 2020

simitt Jun 23, 2020

jalvz Jun 23, 2020

axw Jun 24, 2020

axw commented Jun 23, 2020

jalvz commented Jun 23, 2020

axw commented Jun 24, 2020

axw left a comment

axw Jun 25, 2020

axw Jun 25, 2020

New tracer #3836

New tracer #3836

Conversation

jalvz commented Jun 2, 2020 • edited Loading

Motivation/summary

Checklist

How to test these changes

Related issues

apmmachine commented Jun 3, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Steps errors

jalvz commented Jun 18, 2020

axw commented Jun 23, 2020

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

axw commented Jun 23, 2020

jalvz commented Jun 23, 2020

axw commented Jun 24, 2020

axw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalvz commented Jun 2, 2020 •

edited

Loading

apmmachine commented Jun 3, 2020 •

edited

Loading