Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement module for pgx/v5 #1364

Merged
merged 32 commits into from
Jan 25, 2023
Merged

feat: implement module for pgx/v5 #1364

merged 32 commits into from
Jan 25, 2023

Conversation

gvencadze
Copy link
Contributor

@gvencadze gvencadze commented Dec 18, 2022

This module have some improvements that's was released with pgx/v5, such as detailed batch and connect traces

Closes: #1345

Batch example with connect trace:
example_1

@apmmachine
Copy link
Contributor

apmmachine commented Dec 18, 2022

💔 Tests Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-01-24T22:54:13.337+0000

  • Duration: 49 min 56 sec

Test stats 🧪

Test Results
Failed 6
Passed 7831
Skipped 205
Total 8042

Test errors 6

Expand to view the tests failures

More OS / OSX / [build failed] – apmcloudutil
    Expand to view the error details

     Failed 
    

  • no stacktrace
More OS / OSX / [build failed] – apmhostutil
    Expand to view the error details

     Failed 
    

  • no stacktrace
More OS / OSX / [build failed] – apmhttputil
    Expand to view the error details

     Failed 
    

  • no stacktrace
More OS / OSX / [build failed] – apmtest
    Expand to view the error details

     Failed 
    

  • no stacktrace
More OS / OSX / [build failed] – model
    Expand to view the error details

     Failed 
    

    Expand to view the stacktrace

     <autogenerated>:1: invalid instruction: 03606 (/Users/admin/workspace/gent-go_apm-agent-go-mbp_PR-1364/src/go.elastic.co/apm/model/marshal_fastjson.go:553)	PCDATA	$12
    ?   	go.elastic.co/apm/v2/apmconfig	[no test files] 
    

More OS / OSX / [build failed] – v2
    Expand to view the error details

     Failed 
    

  • no stacktrace

Steps errors 2

Expand to view the steps failures

Test
  • Took 0 min 24 sec . View more details here
  • Description: ./scripts/jenkins/test.sh
Recursively delete the current directory from the workspace
  • Took 0 min 18 sec . View more details here
  • Description: script returned exit code 2

🤖 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!)

module/apmpgxv5/batch_tracer.go Show resolved Hide resolved
}

for _, tt := range testcases {
t.Run(tt.name, func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe do parallel tests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps, I don't think it matters in practice, they should be pretty fast.

module/apmpgxv5/query_tracer.go Outdated Show resolved Hide resolved
module/apmpgxv5/query_tracer_test.go Outdated Show resolved Hide resolved
module/apmpgxv5/batch_tracer.go Outdated Show resolved Hide resolved
module/apmpgxv5/connect_tracer.go Outdated Show resolved Hide resolved
@gvencadze gvencadze changed the title feat: make traces more detailed for pgx/v5 [draft]: feat: make traces more detailed for pgx/v5 Dec 19, 2022
@gvencadze gvencadze changed the title [draft]: feat: make traces more detailed for pgx/v5 feat: make traces more detailed for pgx/v5 Dec 19, 2022
@gvencadze
Copy link
Contributor Author

@axw hey, I think module is ready for review, please take a look

@gvencadze gvencadze changed the title feat: make traces more detailed for pgx/v5 feat: implement module for pgx/v5 Dec 20, 2022
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gvencadze! I've left a handful of comments. Someone else may respond later, as I'll be on holidays for a couple of weeks 🎄

module/apmpgxv5/batch_tracer.go Outdated Show resolved Hide resolved
module/apmpgxv5/batch_tracer.go Outdated Show resolved Hide resolved
module/apmpgxv5/span.go Outdated Show resolved Hide resolved
module/apmpgxv5/span.go Outdated Show resolved Hide resolved
module/apmpgxv5/batch_tracer.go Outdated Show resolved Hide resolved
module/apmpgxv5/batch_tracer.go Outdated Show resolved Hide resolved
module/apmpgxv5/query_tracer.go Show resolved Hide resolved
…ment in endSpan function with error type and wrap sql queries using apmsql.QuerySignature
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks pretty good in general

module/apmpgxv5/batch_tracer.go Outdated Show resolved Hide resolved
module/apmpgxv5/batch_tracer_test.go Outdated Show resolved Hide resolved
module/apmpgxv5/batch_tracer_test.go Outdated Show resolved Hide resolved
module/apmpgxv5/batch_tracer.go Outdated Show resolved Hide resolved
module/apmpgxv5/batch_tracer.go Outdated Show resolved Hide resolved
}

for _, tt := range testcases {
t.Run(tt.name, func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps, I don't think it matters in practice, they should be pretty fast.

module/apmpgxv5/connect_tracer_test.go Outdated Show resolved Hide resolved
module/apmpgxv5/copy_tracer_test.go Outdated Show resolved Hide resolved
module/apmpgxv5/query_tracer_test.go Outdated Show resolved Hide resolved
@gvencadze gvencadze requested a review from marclop January 7, 2023 18:09
module/apmpgxv5/query_tracer_test.go Show resolved Hide resolved
var _ pgx.BatchTracer = (*BatchTracer)(nil)

func (b BatchTracer) TraceBatchStart(ctx context.Context, conn *pgx.Conn, _ pgx.TraceBatchStartData) context.Context {
span, spanCtx, ok := startSpan(ctx, apmsql.QuerySignature("BATCH"), batchSpanType, conn.Config(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move "BATCH" to consts?

module/apmpgxv5/batch_tracer_test.go Show resolved Hide resolved
@gvencadze
Copy link
Contributor Author

@marclop re-review pls 🦊

@marclop
Copy link
Contributor

marclop commented Jan 13, 2023

/test

Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, CI is failing due to a few unformatted files, missing headers, I'll commit those changes to the branch, so the CI passes.

Signed-off-by: Marc Lopez Rubio <[email protected]>
@gvencadze
Copy link
Contributor Author

gvencadze commented Jan 18, 2023

@marclop can you take a look why CI fail pls? I didn't find any error related to my code

@axw
Copy link
Member

axw commented Jan 19, 2023

@gvencadze CI is failing for Go versions 1.15, 1.16, and 1.17. They all fail with this error:

[2023-01-13T02:30:46.787Z] ../../../../../pkg/mod/github.com/jackc/pgx/[email protected]/pgtype/builtin_wrappers.go:8:2: package net/netip is not in GOROOT (/var/lib/jenkins/workspace/gent-go_apm-agent-go-mbp_PR-1364/.gimme/versions/go1.15.15.linux.amd64/src/net/netip)

[2023-01-13T02:30:46.787Z] Makefile:33: recipe for target 'check-vet' failed

Looking at the pgx main branch, it says it requires Go 1.18. I guess we'll need to add a go1.18 build constraint for this module.

…irement + set go version to 1.18 in go.mod file
@gvencadze
Copy link
Contributor Author

gvencadze commented Jan 22, 2023

hey @axw, I've added build tags and set go version in go.mod to 1.18

@axw
Copy link
Member

axw commented Jan 24, 2023

go version in go.mod to 1.18

It's not ideal, but for now please set go.mod to 1.15 - this is needed to get CI to pass.

Please ensure doc.go does not have the go1.18 build tag. There needs to be at least one Go file in the package to prevent this error: https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-go%2Fapm-agent-go-mbp/detail/PR-1364/18/pipeline/128

The //go:build syntax isn't understood by Go 1.15, and Go 1.16 complains if you don't have both comments. I think you'll need to revert 2f655ea to prevent this error: https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-go%2Fapm-agent-go-mbp/detail/PR-1364/18/pipeline/126

@apmmachine
Copy link
Contributor

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (60/60) 💚
Files 99.355% (154/155)
Classes 96.317% (340/353)
Methods 90.349% (983/1088)
Lines 82.206% (11448/13926)
Conditionals 100.0% (0/0) 💚

@axw
Copy link
Member

axw commented Jan 25, 2023

Thanks for your contribution @gvencadze! The macOS CI failure appears to be unrelated, so I'll merge this.

@axw axw merged commit e7c4e6a into elastic:main Jan 25, 2023
@axw axw mentioned this pull request Feb 1, 2023
@kruskall kruskall self-assigned this Mar 29, 2023
@kruskall
Copy link
Member

Verified with APM Server (elastic/apm-server@4c1e156):

  • setup a postgre db: docker run --name postgres -p 127.0.0.1:5432:5432 -e POSTGRES_PASSWORD=changeme -e POSTGRES_DB=testdb -d postgres
  • setup es, kibana and apm-server by using tilt in the apm-server repo: tilt up
  • run the main.go demo: ELASTIC_APM_LOG_FILE=stderr ELASTIC_APM_LOG_LEVEL=debug ELASTIC_APM_SERVER_URL=http://localhost:8200/ go run main.go
  • open kibana and look at the transactions in the APM services tab
main.go
package main

import (
	"context"
	"fmt"
	"log"

	"github.com/jackc/pgx/v5"
	"go.elastic.co/apm/module/apmpgxv5/v2"
	"go.elastic.co/apm/v2"
)

func main() {
	if err := run(); err != nil {
		log.Fatal(err)
	}
}

func run() error {
	cfg, err := pgx.ParseConfig("postgres://postgres:changeme@localhost:5432/testdb")
	if err != nil {
		return fmt.Errorf("failed to parse config: %w", err)
	}

	apmpgxv5.Instrument(cfg)

	t := apm.DefaultTracer()
	t.SetExitSpanMinDuration(0)
	t.SetSpanCompressionEnabled(false)
	tx := t.StartTransaction("t5", "manual")
	defer apm.DefaultTracer().Flush(nil)
	defer tx.End()

	ctx := apm.ContextWithTransaction(context.Background(), tx)

	conn, err := pgx.ConnectConfig(ctx, cfg)
	if err != nil {
		return fmt.Errorf("failed to connect config: %w", err)
	}
	defer func() {
		if err := conn.Close(ctx); err != nil {
			log.Fatal(fmt.Errorf("failed to close2: %w", err))
		}
	}()

	if _, err := conn.Exec(ctx, "CREATE TABLE IF NOT EXISTS foo (bar INT)"); err != nil {
		return fmt.Errorf("failed to create table: %w", err)
	}

	batch := &pgx.Batch{}

	for _, query := range []string{
		"SELECT * FROM foo WHERE bar = 1",
	} {
		batch.Queue(query)
	}

	br := conn.SendBatch(ctx, batch)
	if err := br.Close(); err != nil {
		return fmt.Errorf("failed to close: %w", err)
	}

	rows, err := conn.Query(ctx, "SELECT * FROM foo")
	if err != nil {
		return fmt.Errorf("failed to query: %w", err)
	}
	defer rows.Close()

	return nil
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Make traces more detailed if using pgx/v5
6 participants