add APM instrumentation middleware module for Fiber framework #999

blessedvictim · 2021-08-08T15:48:30Z

These changes add middleware to the fiber framework.

Closes #955

cla-checker-service · 2021-08-08T15:48:33Z

💚 CLA has been signed

apmmachine · 2021-08-08T15:52:55Z

💚 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

Start Time: 2021-09-16T06:51:00.261+0000
Duration: 31 min 0 sec
Commit: 4b7e055

Test stats 🧪

Test	Results
Failed	0
Passed	11078
Skipped	268
Total	11346

Trends 🧪

axw · 2021-08-09T01:32:38Z

Thanks for the PR, @blessedvictim! I've taken a quick look over the PR and it looks good. Before I do a full review, can you please:

sign the CLA: https://www.elastic.co/contributor-agreement
add license headers: make update-licenses

blessedvictim · 2021-08-10T08:38:05Z

@axw hi, i already sign CLA and add license headers

axw · 2021-08-10T08:44:48Z

@blessedvictim 2 of your comments are associated with a different email (@sports.ru). Can you please make sure all commits are created with the same email that you used to sign the CLA?

blessedvictim · 2021-08-12T15:18:31Z

@axw hi, there are still unresolved problems ?

axw · 2021-08-13T01:58:43Z

Thanks for those updates @blessedvictim. Nothing blocking now. Someone will hopefully be able to review within the next week. We're just a bit busy at the moment.

simitt

Thank you for the contribution! I do have a couple of remark, but the PR generally looks good.

Can you please add an entry to the supported tech stack doc and also an example showing how to use this module, similar to the fasthttp example.

module/apmfiber/middleware.go

simitt · 2021-08-13T07:55:51Z

module/apmfiber/middleware.go

+// be ignored. If r is nil, all requests will be reported.
+func WithRequestIgnorer(fn apmfasthttp.RequestIgnorerFunc) Option {
+	if fn == nil {
+		panic("fn == nil")


this is not aligned with the comment, but it should be - it should not panic because of an unset ignore function

If r is nil, all requests will be reported.

module/apmfiber/middleware.go

simitt · 2021-08-13T08:30:57Z

module/apmfiber/middleware.go

+	tracer           *apm.Tracer
+	requestIgnorer   apmfasthttp.RequestIgnorerFunc
+	panicPropagation bool
+}


I don't think introducing this type is necesarry, you should be able to use the middleware.

simitt · 2021-08-13T08:44:21Z

module/apmfiber/middleware_test.go

+
+	resp, err := e.Test(req)
+	assert.Nil(t, err)
+	assert.Equal(t, resp.StatusCode, http.StatusInternalServerError)


Can you also test for the recovered error?

misuto · 2021-08-22T19:24:16Z

I'm also interested in this functionality. I could help by fixing the parts brought up in the comments above but i'm not sure how to wrangle the branches to make this as painless as possible. I guess i should branch from blessedvictim:add-apmfiber-module but don't know to where i should target the PR. Which is the preferred way to do this?

axw · 2021-08-26T01:57:32Z

@misuto thanks for the offer. Let's wait for @blessedvictim to respond.

blessedvictim · 2021-08-27T09:22:45Z

it seems the mentioned issues are fixed now

axw

Thanks @blessedvictim, looks great! Just a few, very minor, comments on the code.

Not sure if you saw @simitt's request?

Can you please add an entry to the supported tech stack doc and also an example showing how to use this module, similar to the fasthttp example.

Then there are a couple of other small things that need to be updated before this can be merged, by running make fmt scripts/Dockerfile-testing.

axw · 2021-08-29T03:30:09Z

module/apmfiber/middleware.go

+			tx.Name = string(reqCtx.Method()) + " unknown route"
+		} else {
+			// Workaround for set tx.Name as template path, not absolute
+			tx.Name = string(reqCtx.Method()) + " " + c.Route().Path


Suggested change

tx.Name = string(reqCtx.Method()) + " " + c.Route().Path

tx.Name = string(reqCtx.Method()) + " " + path

axw · 2021-08-29T03:31:21Z

module/apmfiber/middleware.go

+				defer func() {
+					panic(v)
+				}()


Suggested change

defer func() {

panic(v)

}()

defer panic(v)

axw · 2021-08-29T03:44:26Z

module/apmfiber/middleware.go

+//go:build go1.12
+// +build go1.12
+
+package apmfiber


Suggested change

package apmfiber

package apmfiber // import "go.elastic.co/apm/module/apmfiber"

(this is needed to satisfy make check)

axw · 2021-08-29T03:51:01Z

jenkins run the tests please

module/apmfiber/middleware.go

axw · 2021-09-01T09:37:21Z

@blessedvictim please let us know if you would like any help with the remaining requested changes. If you'd like, we can make the changes and push to your branch and merge.

blessedvictim · 2021-09-01T15:24:31Z

@axw added an example and some description for the apmfiber. Seems like all requested changes done, isn't it ?

axw

Doc addition looks good, thanks - just a couple of minor formatting/copy-paste issues.

A couple of other things remaining:

run make scripts/Dockerfile-testing (needed for our CI to pass)
merge master. There was a minor breaking change to the apmfasthttp exported API recently, the order of args to apmfasthttp.StartTransactionWithBody has changed

docs/instrumenting.asciidoc

…n fasthttp)

delete options type restore WithPanicPropagation add tests for WithPanicPropagation add tests for WithRequestIgnorer

blessedvictim · 2021-09-09T14:08:43Z

@axw I notify you that the noticed problems have been fixed. Is there any other blockers ?

axw · 2021-09-10T00:56:20Z

/test

axw

Looks great, thank you very much for your contribution @blessedvictim! Just need to wait for CI to be happy, then we'll merge.

axw · 2021-09-10T00:57:50Z

jenkins run the tests please

module/apmfiber/middleware_test.go

module/apmfiber/go.mod

increase go version in build tag

axw · 2021-09-15T10:18:50Z

Thanks @blessedvictim. There are some test failures on older versions of Go, which I think are probably unrelated to your changes. I'll investigate tomorrow and merge if I can sort out the issues.

axw · 2021-09-16T07:39:32Z

@elasticmachine, run elasticsearch-ci/docs

axw · 2021-09-16T07:49:11Z

Thanks again for your contribution @blessedvictim!

github-actions bot added the agent-go label Aug 8, 2021

blessedvictim force-pushed the add-apmfiber-module branch 2 times, most recently from 80e3173 to e67e92f Compare August 10, 2021 08:35

blessedvictim force-pushed the add-apmfiber-module branch 2 times, most recently from 29c141d to 99a5c79 Compare August 10, 2021 10:25

zube bot added the [zube]: In Review label Aug 12, 2021

simitt requested changes Aug 13, 2021

View reviewed changes

blessedvictim force-pushed the add-apmfiber-module branch 2 times, most recently from 0b4e7bc to e0d5b29 Compare August 26, 2021 20:43

axw reviewed Aug 29, 2021

View reviewed changes

axw reviewed Aug 30, 2021

View reviewed changes

module/apmfiber/middleware.go Outdated Show resolved Hide resolved

blessedvictim force-pushed the add-apmfiber-module branch from d78f1fd to 1abf022 Compare August 30, 2021 06:55

blessedvictim force-pushed the add-apmfiber-module branch 2 times, most recently from 69374ed to 8ce1a9c Compare September 1, 2021 15:21

axw reviewed Sep 2, 2021

View reviewed changes

docs/instrumenting.asciidoc Outdated Show resolved Hide resolved

docs/instrumenting.asciidoc Outdated Show resolved Hide resolved

add APM instrumentation middleware module for Fiber framework(based o…

8c44d0d

…n fasthttp)

blessedvictim added 3 commits September 2, 2021 11:29

add license header

c38f932

now Options noop on nil

7c048e6

delete options type restore WithPanicPropagation add tests for WithPanicPropagation add tests for WithRequestIgnorer

make fmt

ae61808

blessedvictim force-pushed the add-apmfiber-module branch 2 times, most recently from 5427d41 to 648b37a Compare September 2, 2021 08:38

blessedvictim added 2 commits September 2, 2021 11:42

docs

bc62a9c

upd apmfasthttp

92f43ad

blessedvictim force-pushed the add-apmfiber-module branch from 648b37a to 92f43ad Compare September 2, 2021 08:42

axw approved these changes Sep 10, 2021

View reviewed changes

axw enabled auto-merge (squash) September 10, 2021 00:57

axw reviewed Sep 10, 2021

View reviewed changes

module/apmfiber/middleware_test.go Show resolved Hide resolved

module/apmfiber/go.mod Show resolved Hide resolved

add doc.go

42062b9

increase go version in build tag

auto-merge was automatically disabled September 15, 2021 09:38
Head branch was pushed to by a user without write access

Merge branch 'master' into add-apmfiber-module

4b7e055

axw enabled auto-merge (squash) September 16, 2021 06:51

axw disabled auto-merge September 16, 2021 07:48

axw merged commit bc8b25e into elastic:master Sep 16, 2021

zube bot added [zube]: Done and removed [zube]: In Review labels Sep 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add APM instrumentation middleware module for Fiber framework #999

add APM instrumentation middleware module for Fiber framework #999

blessedvictim commented Aug 8, 2021 •

edited by axw

Loading

cla-checker-service bot commented Aug 8, 2021 •

edited

Loading

apmmachine commented Aug 8, 2021 •

edited

Loading

Build stats

Test stats 🧪

Trends 🧪

axw commented Aug 9, 2021

blessedvictim commented Aug 10, 2021

axw commented Aug 10, 2021

blessedvictim commented Aug 12, 2021

axw commented Aug 13, 2021

simitt left a comment

simitt Aug 13, 2021

simitt Aug 13, 2021

blessedvictim Aug 26, 2021

simitt Aug 13, 2021

misuto commented Aug 22, 2021

axw commented Aug 26, 2021

blessedvictim commented Aug 27, 2021

axw left a comment

axw Aug 29, 2021

axw Aug 29, 2021

axw Aug 29, 2021

axw commented Aug 29, 2021

axw commented Sep 1, 2021

blessedvictim commented Sep 1, 2021

axw left a comment

blessedvictim commented Sep 9, 2021

axw commented Sep 10, 2021

axw left a comment

axw commented Sep 10, 2021

axw commented Sep 15, 2021

axw commented Sep 16, 2021

axw commented Sep 16, 2021

	tx.Name = string(reqCtx.Method()) + " " + c.Route().Path
	tx.Name = string(reqCtx.Method()) + " " + path

	package apmfiber
	package apmfiber // import "go.elastic.co/apm/module/apmfiber"

add APM instrumentation middleware module for Fiber framework #999

add APM instrumentation middleware module for Fiber framework #999

Conversation

blessedvictim commented Aug 8, 2021 • edited by axw Loading

cla-checker-service bot commented Aug 8, 2021 • edited Loading

apmmachine commented Aug 8, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

axw commented Aug 9, 2021

blessedvictim commented Aug 10, 2021

axw commented Aug 10, 2021

blessedvictim commented Aug 12, 2021

axw commented Aug 13, 2021

simitt left a comment

Choose a reason for hiding this comment

simitt Aug 13, 2021

Choose a reason for hiding this comment

simitt Aug 13, 2021

Choose a reason for hiding this comment

blessedvictim Aug 26, 2021

Choose a reason for hiding this comment

simitt Aug 13, 2021

Choose a reason for hiding this comment

misuto commented Aug 22, 2021

axw commented Aug 26, 2021

blessedvictim commented Aug 27, 2021

axw left a comment

Choose a reason for hiding this comment

axw Aug 29, 2021

Choose a reason for hiding this comment

axw Aug 29, 2021

Choose a reason for hiding this comment

axw Aug 29, 2021

Choose a reason for hiding this comment

axw commented Aug 29, 2021

axw commented Sep 1, 2021

blessedvictim commented Sep 1, 2021

axw left a comment

Choose a reason for hiding this comment

blessedvictim commented Sep 9, 2021

axw commented Sep 10, 2021

axw left a comment

Choose a reason for hiding this comment

axw commented Sep 10, 2021

axw commented Sep 15, 2021

axw commented Sep 16, 2021

axw commented Sep 16, 2021

blessedvictim commented Aug 8, 2021 •

edited by axw

Loading

cla-checker-service bot commented Aug 8, 2021 •

edited

Loading

apmmachine commented Aug 8, 2021 •

edited

Loading