add span destination #1123

ivybridge-3c33 · 2021-09-21T04:57:15Z

Add span destination for redisv8 hook to support elastic apm services map.

cla-checker-service · 2021-09-21T04:57:18Z

💚 CLA has been signed

apmmachine · 2021-09-21T05:01:42Z

💚 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

Duration: 36 min 45 sec

🤖 GitHub comments

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

/test : Re-trigger the build.
run benchmark tests : Run the benchmark test.

axw · 2021-09-21T06:15:12Z

@ivybridge-3c33 thanks for the PR!

A few initial comments:

Please sign the CLA. We cannot accept any contributions without this.
We recently deprecated the service destination name; only Resource needs to be set.
Resource will be auto-inferred for "exit spans", introduced in Infer destination service resource adapt public api #1003

Regarding that last point, I think the more natural way to get the destination set would be to change the apm.StartSpan calls to apm.StartSpanOptions, and pass in apm.SpanOptions{ExitSpan: true}. Then the calls to span.Context.SetDestinationService are not needed.

This reverts commit 88d440c.

ivybridge-3c33 · 2021-09-22T07:57:44Z

@ivybridge-3c33 thanks for the PR!

A few initial comments:

Please sign the CLA. We cannot accept any contributions without this.

We recently deprecated the service destination name; only Resource needs to be set.

Resource will be auto-inferred for "exit spans", introduced in Infer destination service resource adapt public api #1003

Regarding that last point, I think the more natural way to get the destination set would be to change the apm.StartSpan calls to apm.StartSpanOptions, and pass in apm.SpanOptions{ExitSpan: true}. Then the calls to span.Context.SetDestinationService are not needed.

thank you @axw
It's ExitSpan is undefined on v1.13.1
I've set destination service because of redis not show up on services map in kibana
how to fix this and next release, Does ExitSpan will support to this issue?

axw · 2021-09-23T02:04:37Z

@ivybridge-3c33 maybe you already saw, but v1.14.0 was just released which includes this change: https://github.com/elastic/apm-agent-go/releases/tag/v1.14.0

If you set ExitSpan: true then redis should show up on the service map. I think we should update the instrumentation module to do this by default. If you would like to open a new PR we would be happy to review that change - otherwise we can get to it later.

ivybridge-3c33 · 2021-09-23T06:59:56Z

@ivybridge-3c33 maybe you already saw, but v1.14.0 was just released which includes this change: https://github.com/elastic/apm-agent-go/releases/tag/v1.14.0

If you set ExitSpan: true then redis should show up on the service map. I think we should update the instrumentation module to do this by default. If you would like to open a new PR we would be happy to review that change - otherwise we can get to it later.

I'll try at latest version first,
Thank you for your support :)

ivybridge-3c33 · 2021-09-23T08:27:41Z

@axw ExitSpan still not show up on the service map

marclop · 2021-09-28T10:51:57Z

@ivybridge-3c33 I tested starting the span with ExitSpan: true and it worked fine for me and was able to assert that the span.Context.Destination.Service.Resource is set to "redis" in the unit tests. My local changes are:

diff --git a/module/apmgoredisv8/hook.go b/module/apmgoredisv8/hook.go
index d524c7b..907c061 100644
--- a/module/apmgoredisv8/hook.go
+++ b/module/apmgoredisv8/hook.go
@@ -40,7 +40,9 @@ func NewHook() redis.Hook {
 
 // BeforeProcess initiates the span for the redis cmd
 func (r *hook) BeforeProcess(ctx context.Context, cmd redis.Cmder) (context.Context, error) {
-	_, ctx = apm.StartSpan(ctx, getCmdName(cmd), "db.redis")
+	_, ctx = apm.StartSpanOptions(ctx, getCmdName(cmd), "db.redis", apm.SpanOptions{
+		ExitSpan: true,
+	})
 	return ctx, nil
 }
 
@@ -63,7 +65,9 @@ func (r *hook) BeforeProcessPipeline(ctx context.Context, cmds []redis.Cmder) (c
 		cmdNameBuf.WriteString(getCmdName(cmd))
 	}
 
-	_, ctx = apm.StartSpan(ctx, cmdNameBuf.String(), "db.redis")
+	_, ctx = apm.StartSpanOptions(ctx, cmdNameBuf.String(), "db.redis", apm.SpanOptions{
+		ExitSpan: true,
+	})
 	return ctx, nil
 }
 
diff --git a/module/apmgoredisv8/hook_test.go b/module/apmgoredisv8/hook_test.go
index d76e0f3..60e836e 100644
--- a/module/apmgoredisv8/hook_test.go
+++ b/module/apmgoredisv8/hook_test.go
@@ -74,12 +74,15 @@ func TestHook(t *testing.T) {
 			assert.Equal(t, "PING", spans[0].Name)
 			assert.Equal(t, "db", spans[0].Type)
 			assert.Equal(t, "redis", spans[0].Subtype)
+			assert.Equal(t, "redis", spans[0].Context.Destination.Service.Resource)
 			assert.Equal(t, "GET", spans[1].Name)
 			assert.Equal(t, "db", spans[1].Type)
 			assert.Equal(t, "redis", spans[1].Subtype)
+			assert.Equal(t, "redis", spans[2].Context.Destination.Service.Resource)
 			assert.Equal(t, "(empty command)", spans[2].Name)
 			assert.Equal(t, "db", spans[2].Type)
 			assert.Equal(t, "redis", spans[2].Subtype)
+			assert.Equal(t, "redis", spans[2].Context.Destination.Service.Resource)
 		})
 	}
 }
@@ -102,6 +105,7 @@ func TestHookPipeline(t *testing.T) {
 			assert.Equal(t, "GET, SET, GET, (empty command)", spans[0].Name)
 			assert.Equal(t, "db", spans[0].Type)
 			assert.Equal(t, "redis", spans[0].Subtype)
+			assert.Equal(t, "redis", spans[0].Context.Destination.Service.Resource)
 		})
 	}
 }
@@ -132,6 +136,7 @@ func TestHookTxPipeline(t *testing.T) {
 			}
 			assert.Equal(t, "db", spans[0].Type)
 			assert.Equal(t, "redis", spans[0].Subtype)
+			assert.Equal(t, "redis", spans[0].Context.Destination.Service.Resource)
 		})
 	}
 }

Updating your PR to only use the apm.SpanOptions{ExitSpan: true} should do it without needing to manually set the destination service resource explicitly.

ivybridge-3c33 · 2021-09-30T08:32:19Z

@ivybridge-3c33 I tested starting the span with ExitSpan: true and it worked fine for me and was able to assert that the span.Context.Destination.Service.Resource is set to "redis" in the unit tests. My local changes are:

diff --git a/module/apmgoredisv8/hook.go b/module/apmgoredisv8/hook.go
index d524c7b..907c061 100644
--- a/module/apmgoredisv8/hook.go
+++ b/module/apmgoredisv8/hook.go
@@ -40,7 +40,9 @@ func NewHook() redis.Hook {
 
 // BeforeProcess initiates the span for the redis cmd
 func (r *hook) BeforeProcess(ctx context.Context, cmd redis.Cmder) (context.Context, error) {
-	_, ctx = apm.StartSpan(ctx, getCmdName(cmd), "db.redis")
+	_, ctx = apm.StartSpanOptions(ctx, getCmdName(cmd), "db.redis", apm.SpanOptions{
+		ExitSpan: true,
+	})
 	return ctx, nil
 }
 
@@ -63,7 +65,9 @@ func (r *hook) BeforeProcessPipeline(ctx context.Context, cmds []redis.Cmder) (c
 		cmdNameBuf.WriteString(getCmdName(cmd))
 	}
 
-	_, ctx = apm.StartSpan(ctx, cmdNameBuf.String(), "db.redis")
+	_, ctx = apm.StartSpanOptions(ctx, cmdNameBuf.String(), "db.redis", apm.SpanOptions{
+		ExitSpan: true,
+	})
 	return ctx, nil
 }
 
diff --git a/module/apmgoredisv8/hook_test.go b/module/apmgoredisv8/hook_test.go
index d76e0f3..60e836e 100644
--- a/module/apmgoredisv8/hook_test.go
+++ b/module/apmgoredisv8/hook_test.go
@@ -74,12 +74,15 @@ func TestHook(t *testing.T) {
 			assert.Equal(t, "PING", spans[0].Name)
 			assert.Equal(t, "db", spans[0].Type)
 			assert.Equal(t, "redis", spans[0].Subtype)
+			assert.Equal(t, "redis", spans[0].Context.Destination.Service.Resource)
 			assert.Equal(t, "GET", spans[1].Name)
 			assert.Equal(t, "db", spans[1].Type)
 			assert.Equal(t, "redis", spans[1].Subtype)
+			assert.Equal(t, "redis", spans[2].Context.Destination.Service.Resource)
 			assert.Equal(t, "(empty command)", spans[2].Name)
 			assert.Equal(t, "db", spans[2].Type)
 			assert.Equal(t, "redis", spans[2].Subtype)
+			assert.Equal(t, "redis", spans[2].Context.Destination.Service.Resource)
 		})
 	}
 }
@@ -102,6 +105,7 @@ func TestHookPipeline(t *testing.T) {
 			assert.Equal(t, "GET, SET, GET, (empty command)", spans[0].Name)
 			assert.Equal(t, "db", spans[0].Type)
 			assert.Equal(t, "redis", spans[0].Subtype)
+			assert.Equal(t, "redis", spans[0].Context.Destination.Service.Resource)
 		})
 	}
 }
@@ -132,6 +136,7 @@ func TestHookTxPipeline(t *testing.T) {
 			}
 			assert.Equal(t, "db", spans[0].Type)
 			assert.Equal(t, "redis", spans[0].Subtype)
+			assert.Equal(t, "redis", spans[0].Context.Destination.Service.Resource)
 		})
 	}
 }

Updating your PR to only use the apm.SpanOptions{ExitSpan: true} should do it without needing to manually set the destination service resource explicitly.

thank you very much it's work, now my pr updated please review

axw

LGTM, thanks @ivybridge-3c33 !

axw · 2021-10-04T02:38:51Z

/test

add span destination

88d440c

github-actions bot added the agent-go label Sep 21, 2021

ivybridge-3c33 added 2 commits September 22, 2021 14:22

Revert "add span destination"

55d5993

This reverts commit 88d440c.

change start span to start span option

09a264a

ivybridge-3c33 closed this Sep 22, 2021

ivybridge-3c33 added 2 commits September 23, 2021 14:09

Merge branch 'elastic:master' into master

28ac4fa

add destination

20008aa

ivybridge-3c33 reopened this Sep 23, 2021

ivybridge-3c33 added 2 commits September 30, 2021 15:29

remove set destination name and set ExitSpan to true

185fd0c

remove get cmd name after processpipeline

cfdc790

Merge branch 'master' into master

24049d7

axw approved these changes Oct 4, 2021

View reviewed changes

axw merged commit 4829c00 into elastic:master Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add span destination #1123

add span destination #1123

ivybridge-3c33 commented Sep 21, 2021

cla-checker-service bot commented Sep 21, 2021 •

edited

Loading

apmmachine commented Sep 21, 2021 •

edited

Loading

Build stats

axw commented Sep 21, 2021

ivybridge-3c33 commented Sep 22, 2021 •

edited

Loading

axw commented Sep 23, 2021

ivybridge-3c33 commented Sep 23, 2021

ivybridge-3c33 commented Sep 23, 2021

marclop commented Sep 28, 2021

ivybridge-3c33 commented Sep 30, 2021

axw left a comment

axw commented Oct 4, 2021

add span destination #1123

add span destination #1123

Conversation

ivybridge-3c33 commented Sep 21, 2021

cla-checker-service bot commented Sep 21, 2021 • edited Loading

apmmachine commented Sep 21, 2021 • edited Loading

💚 Build Succeeded

Build stats

🤖 GitHub comments

axw commented Sep 21, 2021

ivybridge-3c33 commented Sep 22, 2021 • edited Loading

axw commented Sep 23, 2021

ivybridge-3c33 commented Sep 23, 2021

ivybridge-3c33 commented Sep 23, 2021

marclop commented Sep 28, 2021

ivybridge-3c33 commented Sep 30, 2021

axw left a comment

Choose a reason for hiding this comment

axw commented Oct 4, 2021

cla-checker-service bot commented Sep 21, 2021 •

edited

Loading

apmmachine commented Sep 21, 2021 •

edited

Loading

ivybridge-3c33 commented Sep 22, 2021 •

edited

Loading