add s3 session wrapper #927

stuartnelson3 · 2021-04-06T11:10:30Z

tracing for #881

note:

this is still a draft. I have a few questions which I'll call out specifically below.

module/apms3session/session.go

module/apms3session/session_test.go

apmmachine · 2021-04-06T11:19:25Z

💚 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 #927 updated
Start Time: 2021-04-08T12:30:03.461+0000
Duration: 30 min 27 sec
Commit: d2cc381

Test stats 🧪

Test	Results
Failed	0
Passed	10087
Skipped	266
Total	10353

Trends 🧪

I had to hunt around for this, and the first place I looked was the Makefile (where I would expect code generation to occur).

Makefile

stuartnelson3 · 2021-04-06T12:51:52Z

note: default make target was updating the deps for all the modules. this has been committed separately in case there's another process to go about doing that.

stuartnelson3 · 2021-04-06T13:05:11Z

seems like make check-modules is failing, but make update-modules doesn't appear to do anything ...

[2021-04-06T12:57:03.799Z] go run scripts/genmod/main.go -check .
[2021-04-06T12:57:05.187Z] # checking go.elastic.co/apm/module/apmhttp
[2021-04-06T12:57:07.097Z] # checking go.elastic.co/apm
[2021-04-06T12:57:07.097Z]  - missing "replace go.elastic.co/apm/module/apmhttp => module/apmhttp"
[2021-04-06T12:57:07.097Z] 2021/04/06 12:57:06 /var/lib/jenkins/workspace/agent-go_apm-agent-go-mbp_PR-927/src/go.elastic.co/apm/go.mod invalid
[2021-04-06T12:57:07.097Z] exit status 1
[2021-04-06T12:57:07.097Z] Makefile:29: recipe for target 'check-modules' failed

axw

Looking good! The main thing I'd like to see is to move this to a more general module that can create spans for arbitrary aws-sdk-go services.

Makefile

go.mod

model/generate.sh

module/apms3session/session.go

module/apms3session/session_test.go

module/apms3session/session.go

There are two names the bucket name can get encoded into the request URL, either as a subdomain prepended to the host, or as the first member in the path. Handle both of these, as we don't have control over which will be used. Even if explicitly set in the config to use the hosted bucket style, it might only be able to do the path style. https://github.com/aws/aws-sdk-go/blob/main/aws/config.go#L118-L127

also, add make target that will update the file if a new module is created.

seems the tests are failing to build on older versions of go

axw

Nearly there, just a few more small things. We'll also need to add docs: docs/supported_tech.asciidoc and docs/instrumenting.asciidoc. Up to you if you want to do it in this PR or in a followup

Makefile

go.mod

module/apmawssdkgo/session.go

spancontext.go

module/apmawssdkgo/session.go

module/apmawssdkgo/session_test.go

This reverts commit 6b84892.

This reverts commit 052125a.

As mentioned in the comment, after 3 attempts I wasn't able to find an S3 request that used the virtual bucket style pathing (which is supposed to be the default). I opted to test the function for now, even though it's private.

If the bucket name is all caps, it forces the path style URL structure; if the bucket name is lowercase, it will default to virtual bucket style. Test that both cases are handled.

stuartnelson3 · 2021-04-08T12:28:17Z

Going to merge this in favor of moving forward on dynamodb and other services, seems like most of the code will be branching within the send() function based on service type. Can circle back in subsequent PRs if there are outstanding issues that still need to be addressed.

axw

Looks great now, thank you!

Add a wrapper around the aws-sdk-go `session.Session` struct which instruments outgoing requests. This currently only correctly traces calls to `s3`; using the session with other services will produce incorrect traces.

add s3 session wrapper

2d657c3

stuartnelson3 requested a review from axw April 6, 2021 11:10

github-actions bot added the agent-go label Apr 6, 2021

stuartnelson3 commented Apr 6, 2021

View reviewed changes

module/apms3session/session.go Outdated Show resolved Hide resolved

stuartnelson3 commented Apr 6, 2021

View reviewed changes

module/apms3session/session_test.go Outdated Show resolved Hide resolved

stuartnelson3 added 2 commits April 6, 2021 13:59

generate new json marhaler for cloud struct

2381115

add json generation to makefile

b9c4e1b

I had to hunt around for this, and the first place I looked was the Makefile (where I would expect code generation to occur).

stuartnelson3 commented Apr 6, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

stuartnelson3 added 3 commits April 6, 2021 14:30

use correct assert lib

68bdbf2

Add comment for exported apms3session.WrapSession

130e825

go.{mod,sum} updates

6b84892

stuartnelson3 marked this pull request as ready for review April 6, 2021 12:50

axw requested changes Apr 7, 2021

View reviewed changes

stuartnelson3 added 11 commits April 7, 2021 12:34

comments

fa3cbb3

Merge branch 'master' into apms3client

0cfef9b

update handler names

4d8d29b

return hostname from func

8f05882

Use correct upcased service ID for span name

d2ccc77

update dockerfile-testing

a00fdcd

also, add make target that will update the file if a new module is created.

make check

052125a

try naming directories correctly

474fe5f

Add doc.go

fb2e0e8

try setting build constraints

561ecc0

seems the tests are failing to build on older versions of go

axw reviewed Apr 8, 2021

View reviewed changes

module/apmawssdkgo/session_test.go Show resolved Hide resolved

stuartnelson3 added 11 commits April 8, 2021 10:13

comments

0ddc6fb

Revert "go.{mod,sum} updates"

e80e83f

This reverts commit 6b84892.

go.sum update

fd06c67

update instrumenting and supported tech docs

9c85bfe

Revert "make check"

6f5b801

This reverts commit 052125a.

revert changes to go.{mod,sum}

98375e0

add test for getBucketName

1e57acf

As mentioned in the comment, after 3 attempts I wasn't able to find an S3 request that used the virtual bucket style pathing (which is supposed to be the default). I opted to test the function for now, even though it's private.

use ok instead of prs

a676efa

Test parsing bucketName from AWS S3 req url

40ea193

If the bucket name is all caps, it forces the path style URL structure; if the bucket name is lowercase, it will default to virtual bucket style. Test that both cases are handled.

add region if not nil

701bfcc

Add comment about supported services

c864a6e

fix struct comment

d2cc381

axw approved these changes Apr 8, 2021

View reviewed changes

stuartnelson3 merged commit f62afdf into elastic:master Apr 8, 2021

stuartnelson3 deleted the apms3session branch April 8, 2021 14:23

stuartnelson3 mentioned this pull request Apr 8, 2021

Instrument DynamoDB calls #928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add s3 session wrapper #927

add s3 session wrapper #927

stuartnelson3 commented Apr 6, 2021

apmmachine commented Apr 6, 2021 •

edited

Loading

Build stats

Test stats 🧪

Trends 🧪

stuartnelson3 commented Apr 6, 2021

stuartnelson3 commented Apr 6, 2021

axw left a comment

axw left a comment

stuartnelson3 commented Apr 8, 2021

axw left a comment

add s3 session wrapper #927

add s3 session wrapper #927

Conversation

stuartnelson3 commented Apr 6, 2021

apmmachine commented Apr 6, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

stuartnelson3 commented Apr 6, 2021

stuartnelson3 commented Apr 6, 2021

axw left a comment

Choose a reason for hiding this comment

axw left a comment

Choose a reason for hiding this comment

stuartnelson3 commented Apr 8, 2021

axw left a comment

Choose a reason for hiding this comment

apmmachine commented Apr 6, 2021 •

edited

Loading