Added support for go-pg v10 #857

macnibblet · 2020-12-07T06:01:35Z

No description provided.

apmmachine · 2020-12-07T06:05:34Z

💚 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 #857 updated
Start Time: 2020-12-07T11:00:21.725+0000
Duration: 23 min 12 sec

Test stats 🧪

Test	Results
Failed	0
Passed	7638
Skipped	188
Total	7826

axw

Thanks @macnibblet! LGTM. Can you please run go generate in the scripts directory? That is needed to update Dockerfile-testing.

macnibblet · 2020-12-07T07:18:44Z

@axw Tests are passing it Jenkins but it's reporting that it failed, not sure whats wrong here.

axw · 2020-12-07T07:22:32Z

module/apmgopgv10/go.mod

@@ -0,0 +1,14 @@
+module go.elastic.co/apm/module/apmgopg


Suggested change

module go.elastic.co/apm/module/apmgopg

module go.elastic.co/apm/module/apmgopgv10

Sorry, missed this one before! This should fix the CI failures.

Ah, my bad I missed it too.

Didn't seam to help

axw · 2020-12-07T08:12:39Z

jenkins run the tests please

module/apmgopgv10/doc.go

module/apmgopgv10/hook_test.go

Co-authored-by: Andrew Wilkins <axwalk@gmail.com>

codecov-io · 2020-12-07T10:00:09Z

Codecov Report

Merging #857 (e00d617) into master (d72ef83) will decrease coverage by 0.04%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master     #857      +/-   ##
==========================================
- Coverage   83.83%   83.78%   -0.05%     
==========================================
  Files         141      141              
  Lines        7849     7856       +7     
==========================================
+ Hits         6580     6582       +2     
- Misses        863      867       +4     
- Partials      406      407       +1

Impacted Files	Coverage Δ
apmtest/configwatcher.go	`100.00% <ø> (ø)`
apmtest/discard.go	`80.00% <ø> (ø)`
apmtest/env.go	`100.00% <ø> (ø)`
apmtest/httpsuite.go	`100.00% <ø> (ø)`
apmtest/recorder.go	`77.77% <ø> (ø)`
apmtest/recordlogger.go	`87.50% <ø> (ø)`
apmtest/testlogger.go	`66.66% <ø> (ø)`
apmtest/withtransaction.go	`100.00% <ø> (ø)`
breakdown.go	`95.38% <ø> (ø)`
builtin_metrics.go	`81.81% <ø> (ø)`
... and 102 more

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 b228473...e00d617. Read the comment docs.

axw

Now that the other CI issues are sorted, there's a real issue with the code showing up. The hooks API has changed a bit in v10, and requires some changes to hooks.go.

If you'd like to iterate locally, you can run the integration tests like:

scripts/docker-compose-testing build go-agent-tests
scripts/docker-compose-testing run -T --rm go-agent-tests bash -c 'cd module/apmgopgv10 && go test -v'

axw · 2020-12-07T10:17:00Z

module/apmgopgv10/hook.go

+	qh := &queryHook{}
+	switch qh := ((interface{})(qh)).(type) {
+	case pg.QueryHook:
+		db.AddQueryHook(qh)
+		return nil
+	}
+	return errors.New("cannot instrument pg.DB, does not implement required interface")


Suggested change

qh := &queryHook{}

switch qh := ((interface{})(qh)).(type) {

case pg.QueryHook:

db.AddQueryHook(qh)

return nil

}

return errors.New("cannot instrument pg.DB, does not implement required interface")

db.AddQueryHook(&queryHook{})

return nil

IIRC, this interface approach existed because the AddQueryHook method didn't exist at the beginning of the v9 cycle. AddQueryHook exists in all versions of v10, so we should just call the method directly -- then we get static type checking back again.

Makes sense, sorry for the screw-up. I was replacing the wrong path locally and still using the old version when I tested it.

axw · 2020-12-07T10:19:12Z

module/apmgopgv10/hook.go

+type queryHook struct{}
+
+// BeforeQuery initiates the span for the database query
+func (qh *queryHook) BeforeQuery(evt *pg.QueryEvent) {


Suggested change

func (qh *queryHook) BeforeQuery(evt *pg.QueryEvent) {

func (qh *queryHook) BeforeQuery(ctx context.Context, evt *pg.QueryEvent) (context.Context, error) {

Turns out that the QueryHook interface has changed in v10. Instead of using evt.Stash we should return a context with the span, and extract the span from the context passed into AfterQuery.

axw · 2020-12-07T10:19:24Z

module/apmgopgv10/hook.go

+		sql = []byte(fmt.Sprintf("[go-pg] error: %s", err.Error()))
+	}
+
+	span, _ := apm.StartSpan(evt.DB.Context(), apmsql.QuerySignature(string(sql)), "db.postgresql.query")


Suggested change

span, _ := apm.StartSpan(evt.DB.Context(), apmsql.QuerySignature(string(sql)), "db.postgresql.query")

span, ctx := apm.StartSpan(ctx, apmsql.QuerySignature(string(sql)), "db.postgresql.query")

axw · 2020-12-07T10:19:52Z

module/apmgopgv10/hook.go

+		Instance: database,
+	})
+
+	evt.Stash[elasticApmSpanKey] = span


Suggested change

evt.Stash[elasticApmSpanKey] = span

return ctx, nil

axw · 2020-12-07T10:20:09Z

module/apmgopgv10/hook.go

+const elasticApmSpanKey = "go-apm-agent:span"
+


Suggested change

const elasticApmSpanKey = "go-apm-agent:span"

axw · 2020-12-07T10:21:31Z

module/apmgopgv10/hook.go

+}
+
+// AfterQuery ends the initiated span from BeforeQuery
+func (qh *queryHook) AfterQuery(evt *pg.QueryEvent) {


Suggested change

func (qh *queryHook) AfterQuery(evt *pg.QueryEvent) {

func (qh *queryHook) AfterQuery(ctx context.Context, evt *pg.QueryEvent) error {

axw · 2020-12-07T10:23:07Z

module/apmgopgv10/hook.go

+	span, ok := evt.Stash[elasticApmSpanKey]
+	if !ok {
+		return
+	}
+	if s, ok := span.(*apm.Span); ok {
+		s.End()
+	}


Suggested change

span, ok := evt.Stash[elasticApmSpanKey]

if !ok {

return

}

if s, ok := span.(*apm.Span); ok {

s.End()

}

if span := apm.SpanFromContext(ctx); span != nil {

span.End()

}

return nil

macnibblet · 2020-12-07T10:59:15Z

module/apmgopgv10/hook.go

+
+	sql, err := evt.UnformattedQuery()
+	if err != nil {
+		return ctx, errors.Wrap(err, "failed to generate sql")


@axw I'm not sure about this part, but I don't know what would possibly cause it to fail.

I don't either; this looks fine to me.

macnibblet · 2020-12-07T11:00:37Z

module/apmgopgv10/hook.go

+func Instrument(db *pg.DB) error {
+	db.AddQueryHook(&queryHook{})
+
+	return nil


Do we want to keep this to have the same API as the previous version?

I think so. This minimises the number of changes required to update the instrumentation when upgrading, and leaves the door open for additional checks later.

axw

Thanks for bearing with me and the CI issues! Looks good.

macnibblet · 2020-12-07T13:09:55Z

@axw Can I bother you for a tag :)?

axw · 2020-12-08T00:38:15Z

@macnibblet there's a few issues that I'd like to resolve before creating a new release (https://github.com/elastic/apm-agent-go/milestone/9). That will probably be a few weeks away still. Can you please pin to the latest commit in the mean time?

axw · 2021-01-21T01:10:41Z

@macnibblet happy new year! Sorry for the long wait -- we've just created a new release, v1.10.0, with your changes.

Added support for v10

53b3f55

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

removed v8 from dependency list

c74d89a

axw approved these changes Dec 7, 2020

View reviewed changes

Added the new gopgv10 to the testing setup

dfee614

axw reviewed Dec 7, 2020

View reviewed changes

Added missing v10 suffix for go.mod

c365578

axw reviewed Dec 7, 2020

View reviewed changes

module/apmgopgv10/doc.go Outdated Show resolved Hide resolved

Updated the build tag

b1939f4

axw reviewed Dec 7, 2020

View reviewed changes

module/apmgopgv10/doc.go Outdated Show resolved Hide resolved

module/apmgopgv10/hook_test.go Outdated Show resolved Hide resolved

macnibblet and others added 2 commits December 7, 2020 17:43

Update module/apmgopgv10/doc.go

257a0a8

Co-authored-by: Andrew Wilkins <axwalk@gmail.com>

Update module/apmgopgv10/hook_test.go

e00d617

Co-authored-by: Andrew Wilkins <axwalk@gmail.com>

axw requested changes Dec 7, 2020

View reviewed changes

Actually implement the v10 hook

a15044b

macnibblet commented Dec 7, 2020

View reviewed changes

Clean up

c331f19

macnibblet commented Dec 7, 2020

View reviewed changes

axw approved these changes Dec 7, 2020

View reviewed changes

axw merged commit 46d8c0a into elastic:master Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for go-pg v10 #857

Added support for go-pg v10 #857

macnibblet commented Dec 7, 2020

apmmachine commented Dec 7, 2020 •

edited

Build stats

Test stats 🧪

axw left a comment

macnibblet commented Dec 7, 2020

axw Dec 7, 2020

macnibblet Dec 7, 2020

macnibblet Dec 7, 2020

axw commented Dec 7, 2020

codecov-io commented Dec 7, 2020

axw left a comment

axw Dec 7, 2020

macnibblet Dec 7, 2020

axw Dec 7, 2020

axw Dec 7, 2020

axw Dec 7, 2020

axw Dec 7, 2020

axw Dec 7, 2020

axw Dec 7, 2020

macnibblet Dec 7, 2020

axw Dec 7, 2020

macnibblet Dec 7, 2020

axw Dec 7, 2020

axw left a comment

macnibblet commented Dec 7, 2020

axw commented Dec 8, 2020

axw commented Jan 21, 2021

	module go.elastic.co/apm/module/apmgopg
	module go.elastic.co/apm/module/apmgopgv10

	func (qh queryHook) BeforeQuery(evt pg.QueryEvent) {
	func (qh queryHook) BeforeQuery(ctx context.Context, evt pg.QueryEvent) (context.Context, error) {

	span, _ := apm.StartSpan(evt.DB.Context(), apmsql.QuerySignature(string(sql)), "db.postgresql.query")
	span, ctx := apm.StartSpan(ctx, apmsql.QuerySignature(string(sql)), "db.postgresql.query")

	func (qh queryHook) AfterQuery(evt pg.QueryEvent) {
	func (qh queryHook) AfterQuery(ctx context.Context, evt pg.QueryEvent) error {

Added support for go-pg v10 #857

Added support for go-pg v10 #857

Conversation

macnibblet commented Dec 7, 2020

apmmachine commented Dec 7, 2020 • edited

💚 Build Succeeded

Build stats

Test stats 🧪

axw left a comment

Choose a reason for hiding this comment

macnibblet commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axw commented Dec 7, 2020

codecov-io commented Dec 7, 2020

Codecov Report

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

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

Choose a reason for hiding this comment

axw left a comment

Choose a reason for hiding this comment

macnibblet commented Dec 7, 2020

axw commented Dec 8, 2020

axw commented Jan 21, 2021

apmmachine commented Dec 7, 2020 •

edited