feat(module): add module for tracing SQL queries created by jackc/pgx by gvencadze · Pull Request #1301 · elastic/apm-agent-go

gvencadze · 2022-08-20T14:10:51Z

This module provide enhanced support of jackc/pgx library and it's features such as COPY FROM and BATCH statements

Closes #854

…ction test

…n external API

…p pgx library version up to v4.17 and write comment for error with min required version

…://www.elastic.co/guide/en/apm/agent/go/master/builtin-modules.html"

cla-checker-service · 2022-08-20T14:10:55Z

💚 CLA has been signed

ghost · 2022-08-20T14:15: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

Start Time: 2022-10-24T08:42:28.137+0000
Duration: 58 min 44 sec

Test stats 🧪

Test	Results
Failed	0
Passed	8688
Skipped	215
Total	8903

🤖 GitHub comments

Expand to view the 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.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

axw · 2022-09-08T02:15:40Z

@gvencadze sorry for the silence, this slipped my attention.

Can you please sign the CLA? As there are commits from multiple authors, both you and @EpicStep will need to do this.

gvencadze · 2022-09-13T12:53:21Z

@axw hey, I've signed CLA (tried with both of emails that's linked to account), but nothing changed. Do I need to restart pipeline or something else?

UPD: merge latest changes and CLA check re-runs, now it's ok

axw

This looks great, thanks for the PR! I've left some comments, and in addition to those:

Would you please take a look at adding an integration test? You can adapt the apmsql/pq one here: https://github.com/elastic/apm-agent-go/blob/main/module/apmsql/pq/pq_test.go
Please run make update-licenses update-modules fmt scripts/Dockerfile-testing
Please add vanity import comments to the files, so that make check passes

module/apmpgx/tracer.go

…cepts pgx.ConnConfig. Move duplicate code to separate func called startSpan to make ...Trace functions small and simple. Add type casting checks to avoid instrument runtime panics.

…t with Instrument func for further tests

… and add apmpgx entry to Dockerfile-testing

gvencadze · 2022-09-15T21:06:07Z

@axw hey, I've done what you've write about above:

integration tests are already presented in tracer_test.go, they're located under TestLog
make update-licenses update-modules fmt scripts/Dockerfile-testing - done, but I've seen 400 bad request in stdout (What am I doing wrong?)
vanity import - done

that's stdout after make call:

=== RUN   TestHTTPTransportOptionsEmptyURL
    http_test.go:707: 
        	Error Trace:	http_test.go:707
        	Error:      	Received unexpected error:
        	            	request failed with 400 Bad Request: {"accepted":0,"errors":[{"message":"unexpected EOF"}]}
        	Test:       	TestHTTPTransportOptionsEmptyURL
    http_test.go:708: 
        	Error Trace:	http_test.go:708
        	Error:      	"[]" should have 1 item(s), but has 0
        	Test:       	TestHTTPTransportOptionsEmptyURL

gvencadze · 2022-09-26T07:29:34Z

@axw I've added e2e tests for Query and Copy tracing, only the batch trace remain. Can you take a look? Maybe you will find something else that should be changed/discussed

axw

LGTM! Thank you, this looks great.

axw · 2022-09-27T02:08:07Z

/test

axw · 2022-09-28T00:51:58Z

@gvencadze there's an issue with formatting, CI is failing with this error:

[2022-09-27T17:20:32.075Z] goimports differs:
[2022-09-27T17:20:32.076Z]  - module/apmpgx/e2e_test.go

Can you please make sure make check-full passes locally?

gvencadze · 2022-09-28T03:43:28Z

@gvencadze there's an issue with formatting, CI is failing with this error:
[2022-09-27T17:20:32.075Z] goimports differs:
[2022-09-27T17:20:32.076Z]  - module/apmpgx/e2e_test.go
Can you please make sure make check-full passes locally?

Sure, I’ll take a look

UPD: @axw, I've run make check, and got 400 bad request in http modules tests

gvencadze · 2022-09-28T10:33:50Z

module/apmpgx/tracer.go

+	statement, ok := data["sql"].(string)
+	if !ok {
+		apm.CaptureError(ctx,
+			fmt.Errorf("%w: expect string, got: %T",
+				ErrInvalidType,
+				data["sql"],
+			),
+		).Send()
+		return
+	}


@axw one more question - should I capture and send type casting errors into APM? I didn't find something like this in other modules, but I think it will be good for understanding that something inside module (or its dependency) is broken

There's no precedent and I'm a bit on the fence about it, but I think it's fine to keep it for now at least. There's not really a good alternative at the moment. Generally errors are about the service, rather than about the instrumentation. I suppose ideally there would be some way to log errors about the instrumentation - we can revise it later.

…r according to stored value in data map

ghost · 2022-10-23T11:03:52Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`59/59`)	💚
Files	99.346% (`152/153`)	👍
Classes	96.275% (`336/349`)	👍
Methods	90.49% (`961/1062`)	👍
Lines	82.269% (`11224/13643`)	👎 -0.015
Conditionals	100.0% (`0/0`)	💚

axw · 2022-10-24T00:40:44Z

@gvencadze I see you've sorted out the last of the tests 🎉

One last thing before I merge it: would you please add an entry to CHANGELOG.asciidoc, to the "Unreleased" section?

…section

gvencadze · 2022-10-24T08:41:51Z

@axw done

Also, If we want to create another one module for pgx (but for v5 version), should we open issue and link it with MR or not?
Current module have support of both versions, but I think we can make it better using v5 built-in changes

I'm talking about this new method:
https://pkg.go.dev/github.com/jackc/pgx/v5#CopyFromTracer

They aren't available in V4 version, so I think we should make another one module, something called apmpgxv5 for example

axw · 2022-10-24T10:09:17Z

run elasticsearch-ci/docs

axw · 2022-10-24T10:12:46Z

They aren't available in V4 version, so I think we should make another one module, something called apmpgxv5 for example

I agree, that sounds like the way to go. Yes, please do open a separate issue and reference this PR.

axw · 2022-10-24T10:24:16Z

Thank you very much for your contribution!

EpicStep and others added 17 commits May 8, 2022 12:40

feat: implementation of pgx query tracer

474d35c

feat: add logger support

3c0da44

feat: add copy protocol support

dded6bc

test: add tests for query and copy trace

f183141

test: add table tests to test errors in different cases + add Log fun…

bb32dd4

…ction test

feat: implement batch trace

aecc104

Merge branch 'main' of https://github.com/EpicStep/apm-agent-go

9f83c4b

docs: add godoc

290efa0

feat: add stacktrace to span

79c2de7

test: add stacktrace length check

70a8bb8

refac: make tracer private

6a98667

refac: rename package from apmpgx to apmpgx_test to test usage like a…

6e3a16b

…n external API

test: add t.Parallel() to table tests

fb5784c

Merge branch 'elastic:main' into main

3740a9e

chore(tracer): remove set stacktrace because it's unnecessary and bum…

c74563c

…p pgx library version up to v4.17 and write comment for error with min required version

docs(instrumenting): add description of library that serves at "https…

af23173

…://www.elastic.co/guide/en/apm/agent/go/master/builtin-modules.html"

docs(instrumenting): add example for apmgpx usage without pool

89f329b

github-actions bot added the agent-go label Aug 20, 2022

Merge branch 'main' into main

1d46bab

axw requested changes Sep 15, 2022

View reviewed changes

gvencadze added 5 commits September 15, 2022 23:11

refac: replace NewTracer constructor with Instrument function that ac…

b537c24

…cepts pgx.ConnConfig. Move duplicate code to separate func called startSpan to make ...Trace functions small and simple. Add type casting checks to avoid instrument runtime panics.

test: replace NewTracer call with creating config and then override i…

e25a2fd

…t with Instrument func for further tests

Merge remote-tracking branch 'origin/main'

802c241

docs(ci): add license header, run update-licenses,update-modules, fmt…

2db48bb

… and add apmpgx entry to Dockerfile-testing

fix(ci): add vanity import path

d81e7ee

Merge branch 'main' into main

1923bb5

axw approved these changes Sep 27, 2022

View reviewed changes

Merge branch 'main' into main

3337275

feat(copy): add statement to copy from trace

d511503

gvencadze commented Sep 28, 2022

View reviewed changes

gvencadze and others added 9 commits September 28, 2022 14:46

test(copy): add statement to DatabaseSpanContext into E2E_CopyTrace test

2b78355

Merge remote-tracking branch 'origin/main'

2b9836d

fix(test): revert postgres connection in E2E_CopyTrace

d238aa9

Merge branch 'main' into main

6bfd7f9

Merge branch 'main' into main

4743db8

fix(test): replace interface type cast from []string to pgx.Identifie…

598b289

…r according to stored value in data map

Merge remote-tracking branch 'origin/main'

7db506e

fix(tracer): add type switch for column names that stored in data map

3385ab0

Merge branch 'main' into main

c39338f

gvencadze added 2 commits October 24, 2022 11:40

docs(changelog): add apmpgx module description entry into unreleased …

f393244

…section

Merge remote-tracking branch 'origin/main'

ab89c49

Merge branch 'main' into main

4e7e7b1

axw enabled auto-merge (squash) October 24, 2022 10:14

axw merged commit 4d07ba8 into elastic:main Oct 24, 2022

gvencadze mentioned this pull request Oct 29, 2022

[Feature Request]: Make traces more detailed if using pgx/v5 #1345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(module): add module for tracing SQL queries created by jackc/pgx#1301

feat(module): add module for tracing SQL queries created by jackc/pgx#1301
axw merged 44 commits intoelastic:mainfrom
EpicStep:main

gvencadze commented Aug 20, 2022 •

edited by axw

Loading

cla-checker-service bot commented Aug 20, 2022 •

edited

Loading

ghost commented Aug 20, 2022 •

edited by ghost

Loading

Build stats

Test stats 🧪

axw commented Sep 8, 2022

gvencadze commented Sep 13, 2022 •

edited

Loading

axw left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvencadze commented Sep 15, 2022 •

edited

Loading

gvencadze commented Sep 26, 2022

axw left a comment

axw commented Sep 27, 2022

axw commented Sep 28, 2022

gvencadze commented Sep 28, 2022 •

edited

Loading

gvencadze Sep 28, 2022

axw Sep 28, 2022

ghost commented Oct 23, 2022 •

edited by ghost

Loading

axw commented Oct 24, 2022

gvencadze commented Oct 24, 2022 •

edited

Loading

axw commented Oct 24, 2022

axw commented Oct 24, 2022

axw commented Oct 24, 2022

Labels

3 participants

Conversation

gvencadze commented Aug 20, 2022 • edited by axw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

cla-checker-service bot commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

axw commented Sep 8, 2022

gvencadze commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

axw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvencadze commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

gvencadze commented Sep 26, 2022

axw left a comment

Choose a reason for hiding this comment

axw commented Sep 27, 2022

axw commented Sep 28, 2022

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

Uh oh!

gvencadze Sep 28, 2022

Choose a reason for hiding this comment

axw Sep 28, 2022

Choose a reason for hiding this comment

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

Uh oh!

🌐 Coverage report

axw commented Oct 24, 2022

gvencadze commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

axw commented Oct 24, 2022

axw commented Oct 24, 2022

axw commented Oct 24, 2022

Labels

3 participants

gvencadze commented Aug 20, 2022 •

edited by axw

Loading

cla-checker-service bot commented Aug 20, 2022 •

edited

Loading

ghost commented Aug 20, 2022 •

edited by ghost

Loading

gvencadze commented Sep 13, 2022 •

edited

Loading

gvencadze commented Sep 15, 2022 •

edited

Loading

gvencadze commented Sep 28, 2022 •

edited

Loading

ghost commented Oct 23, 2022 •

edited by ghost

Loading

gvencadze commented Oct 24, 2022 •

edited

Loading