Backward compatibility with svc target and dest resource by lahsivjar · Pull Request #7925 · elastic/apm-server

lahsivjar · 2022-04-21T03:34:27Z

Motivation/summary

Deprecate span.context.destination.service.resource
Infer span.context.service.target.{type, name} from span.context.destination.service.resource if the former not provided

Full spec list available at elastic/apm#627

Checklist

Update CHANGELOG.asciidoc
Update package changelog.yml (only if changes to apmpackage have been made)
Documentation has been updated

How to test these changes

Case 1

Send span.context.service.target.{type, name} via the intake API
Observe the generated span documents to contain service.target.name and service.target.type values

Case 2

Send span.context.destination.service.resource via the intake API along with span.context.service.target.{type, name}
Observe the generated span documents to contain service.target.name and service.target.type values as passed via the intake API

Case 3

Send span.context.destination.service.resource via the intake API WITHOUT span.context.service.target.{type, name} in 3 formats via 3 separate requests, example:
- postgres/testdb
- postgres
- localhost:8080
Observe the generated span document to contain service.target.name and service.target.type values based on the spec. Example based on point 1:
- target.type = postgres and target.name = testdb
- target.type = postgres
- target.type = "" and target.name = localhost:8080

Related issues

Closes #7734

mergify · 2022-04-21T03:34:59Z

This pull request does not have a backport label. Could you fix it @lahsivjar? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-7.x is the label to automatically backport to the 7.x branch.
backport-7./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

ghost · 2022-04-21T04:02:17Z

💔 Tests Failed

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

Expand to view the summary

Build stats

Start Time: 2022-05-18T07:57:30.822+0000
Duration: 28 min 58 sec

Test stats 🧪

Test	Results
Failed	2
Passed	3982
Skipped	13
Total	3997

Test errors

Expand to view the tests failures

`Build and Test / System and Environment Tests / Test_warmup/16_agent_[100_1000_100000]_events – github.com/elastic/apm-server/systemtest/benchtest`

Expand to view the error details

 Failed

Expand to view the stacktrace

 === RUN   Test_warmup/16_agent_[100_1000_100000]_events
    main_test.go:89: 
        	Error Trace:	main_test.go:89
        	Error:      	Received unexpected error:
        	            	13 errors: context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded; context deadline exceeded
        	Test:       	Test_warmup/16_agent_[100_1000_100000]_events
    --- FAIL: Test_warmup/16_agent_[100_1000_100000]_events (80.89s)

`Build and Test / System and Environment Tests / Test_warmup – github.com/elastic/apm-server/systemtest/benchtest`

Expand to view the error details

 Failed

Expand to view the stacktrace

 === RUN   Test_warmup
--- FAIL: Test_warmup (84.58s)

Steps errors

Expand to view the steps failures

`Run Linux tests`

Took 15 min 19 sec . View more details here
Description: ./.ci/scripts/linux-test.sh

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/hey-apm : Run the hey-apm benchmark.
/package : Generate and publish the docker images.
/test windows : Build & tests on Windows.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

lahsivjar · 2022-04-21T05:37:50Z

/test

ghost · 2022-04-28T16:03:35Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`42/42`)	💚
Files	91.878% (`181/197`)	👍
Classes	93.392% (`424/454`)	👍
Methods	89.238% (`1078/1208`)	👍 0.009
Lines	76.915% (`13071/16994`)	👍 0.13
Conditionals	100.0% (`0/0`)	💚

lahsivjar · 2022-05-11T07:13:56Z

/test

stuartnelson3

looks good! could you add a unit test to verify this is being called and doing the right thing within the mapping code path?

SylvainJuge

LGTM on my side, definitely seems to fit the spec. +100 on adding automated tests for that as this kind of mapping tend to break easily over time. As I'm not go-fluent and very familiar with apm-server codebase, don´t block merging this PR on my approval.

I would be able to validate this with an agent once #7924 is merged.

SylvainJuge · 2022-05-11T15:46:40Z

In the Case 3, when not sending the new fields, for the last case, on top of checking that target.name = localhost:8080, we need to check that target.type = "" (empty string). This is important as it allows the UI to query on target.{name,type} and use the empty target.type as an indicator to use the resource for display (might be covered in elastic/kibana#131923)

This should also apply to the general case where the resource can't be parsed to any known format, in this case the field target.type should be set to an empty value.

marclop

I'm also in favor to adding some tests here to ensure it behaves as expected.

marclop

LGTM. Thanks for all the changes

marclop · 2022-05-17T08:30:59Z

Mind adding a changelog entry as well? It seems that's the only thing missing.

lahsivjar · 2022-05-17T08:52:52Z

Mind adding a changelog entry as well? It seems that's the only thing missing.

@marclop Added changelog entry

stuartnelson3

looks good!

lahsivjar · 2022-05-18T07:56:58Z

/test

lahsivjar · 2022-05-18T09:40:23Z

Failing tests are flaky, merging based on this comment

stuartnelson3 · 2022-06-08T12:21:41Z

tested with d920cdd

case 1:

# cat doc1.json | jq .span
{
  "duration": {
    "us": 32592
  },
  "name": "GET /api/types",
  "id": "1234abcdef567895",
  "type": "request"
}

# cat doc1.json | jq .service.target
{
  "name": "myname",
  "type": "abc123"
}

Is this correct? The test case says the data should be available under span.service.target.*, but it's actually under _source.service.target*. Not sure if you meant _source to represent the span document data, or if it's to be nested under span within the document.

case 2: same question; assuming this is correct then 👌

case 3: same question; assuming this is correct then 👌

lahsivjar · 2022-06-09T02:26:03Z

@stuartnelson3 You are correct, it should be _source.service.target.*, I have updated the summary. Thanks for the tests.

stuartnelson3 · 2022-06-13T10:32:25Z

cool, then 👍 confirmed

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Apr 21, 2022

lahsivjar force-pushed the 7734_backward_compatibility branch from b8366d1 to c7ec339 Compare April 21, 2022 05:36

lahsivjar force-pushed the 7734_backward_compatibility branch from c7ec339 to 35a7111 Compare May 11, 2022 06:57

lahsivjar force-pushed the 7734_backward_compatibility branch from 35a7111 to 05c369f Compare May 11, 2022 08:12

Backward compatibility with svc target and dest resource

2546636

lahsivjar force-pushed the 7734_backward_compatibility branch from 05c369f to 2546636 Compare May 11, 2022 09:01

lahsivjar marked this pull request as ready for review May 11, 2022 09:33

lahsivjar requested review from a team and SylvainJuge May 11, 2022 09:34

lahsivjar mentioned this pull request May 11, 2022

Backend dependencies granularity: keep backwards compatibility #7734

Closed

stuartnelson3 suggested changes May 11, 2022

View reviewed changes

SylvainJuge reviewed May 11, 2022

View reviewed changes

marclop reviewed May 12, 2022

View reviewed changes

Comment thread model/modeldecoder/v2/decoder.go Outdated

Comment thread model/modeldecoder/v2/decoder.go Outdated

Comment thread model/modeldecoder/v2/decoder.go Outdated

Refactor map from resource to svc target

34494cd

simitt approved these changes May 16, 2022

View reviewed changes

Comment thread model/modeldecoder/v2/decoder.go Outdated

lahsivjar added 3 commits May 17, 2022 12:15

Add test for decode map to span

2d93a18

Merge branch 'main' into 7734_backward_compatibility

7e4cc4b

Refactor service target regex

dd08ac1

lahsivjar requested review from marclop and stuartnelson3 May 17, 2022 04:41

lahsivjar added 2 commits May 17, 2022 13:00

Refactor subtest names

0137b81

Fix formatting for test data

0b57346

marclop approved these changes May 17, 2022

View reviewed changes

lahsivjar added 2 commits May 17, 2022 16:50

Add changelog

9994e3e

Merge branch 'main' into 7734_backward_compatibility

cb6fa50

stuartnelson3 approved these changes May 18, 2022

View reviewed changes

Merge branch 'main' into 7734_backward_compatibility

d453206

lahsivjar enabled auto-merge (squash) May 18, 2022 07:07

Merge branch 'main' into 7734_backward_compatibility

75ace42

lahsivjar disabled auto-merge May 18, 2022 09:18

lahsivjar merged commit 6dbbc4c into main May 18, 2022

lahsivjar deleted the 7734_backward_compatibility branch May 18, 2022 09:42

simitt mentioned this pull request May 25, 2022

Supporting new Backend Dependencies fields #7656

Closed

3 tasks

stuartnelson3 self-assigned this Jun 8, 2022

Conversation

lahsivjar commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation/summary

Checklist

How to test these changes

Related issues

Uh oh!

mergify bot commented Apr 21, 2022

Uh oh!

ghost commented Apr 21, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Tests Failed

Build stats

Test stats 🧪

Test errors

Build and Test / System and Environment Tests / Test_warmup/16_agent_[100_1000_100000]_events – github.com/elastic/apm-server/systemtest/benchtest

Build and Test / System and Environment Tests / Test_warmup – github.com/elastic/apm-server/systemtest/benchtest

Steps errors

Run Linux tests

🤖 GitHub comments

Uh oh!

lahsivjar commented Apr 21, 2022

Uh oh!

ghost commented Apr 28, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

lahsivjar commented May 11, 2022

Uh oh!

stuartnelson3 left a comment

Choose a reason for hiding this comment

Uh oh!

SylvainJuge left a comment

Choose a reason for hiding this comment

Uh oh!

SylvainJuge commented May 11, 2022

Uh oh!

marclop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marclop left a comment

Choose a reason for hiding this comment

Uh oh!

marclop commented May 17, 2022

Uh oh!

lahsivjar commented May 17, 2022

Uh oh!

stuartnelson3 left a comment

Choose a reason for hiding this comment

Uh oh!

lahsivjar commented May 18, 2022

Uh oh!

lahsivjar commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartnelson3 commented Jun 8, 2022

Uh oh!

lahsivjar commented Jun 9, 2022

Uh oh!

stuartnelson3 commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lahsivjar commented Apr 21, 2022 •

edited

Loading

ghost commented Apr 21, 2022 •

edited by ghost

Loading

`Build and Test / System and Environment Tests / Test_warmup/16_agent_[100_1000_100000]_events – github.com/elastic/apm-server/systemtest/benchtest`

`Build and Test / System and Environment Tests / Test_warmup – github.com/elastic/apm-server/systemtest/benchtest`

`Run Linux tests`

ghost commented Apr 28, 2022 •

edited by ghost

Loading

lahsivjar commented May 18, 2022 •

edited

Loading