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

Jump to bottom

Go lambda instrumentation enhancements #1413

Merged
merged 33 commits into from Nov 12, 2021
Merged

Go lambda instrumentation enhancements #1413

merged 33 commits into from Nov 12, 2021

Conversation

bhautikpip
Copy link
Contributor

  1. This PR introduces Go Lambda instrumentation enhancements to make xrayconfig package more useful.

bhautikpip and others added 24 commits February 4, 2021 16:21
@bhautikpip
added failure scenario when getting container fails
dcc01de
@bhautikpip
fix test case failure
1dd54d9
@bhautikpip
add changelog
03f6fb4
@bhautikpip
Merge branch 'main' into main
5919be5
@Aneurysm9
Merge branch 'main' into main
ebd45b4
@bhautikpip
fix ecs resource detector bug
e1ff7d0
@bhautikpip
Merge branch 'main' of https://github.com/bhautikpip/opentelemetry-go…
041d9b8 
…-contrib into main
@bhautikpip
fix struct name as per golint suggestion
e12a4b1
@bhautikpip
fix merge conflict
e906d2a
@bhautikpip
minor changes
86c04d1
@bhautikpip
added NewResourceDetector func and interface assertions
047f5d0
@bhautikpip
fix golint failure
21db8fa
@bhautikpip
minor changes to address review comments
5204d27
@MrAlias
Merge branch 'main' into main
c9c1bca
@bhautikpip
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
c956d4b 
…y-go-contrib into main
@bhautikpip
Merge branch 'main' of https://github.com/bhautikpip/opentelemetry-go…
12c6c74 
…-contrib into main
@bhautikpip
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
e042a6f 
…y-go-contrib into main
@bhautikpip
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
1f6b745 
…y-go-contrib into main
@bhautikpip
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
ada0137 
…y-go-contrib into main
@bhautikpip
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
9c6a430 
…y-go-contrib into main
@bhautikpip
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
79d6d15 
…y-go-contrib into main
@bhautikpip
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
e21f9a6 
…y-go-contrib into main
@bhautikpip
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
2e0f29a 
…y-go-contrib into main
@bhautikpip
lambda go instrumentation enhancements
f30b844
@codecov
Copy link

codecov bot commented Nov 8, 2021 •
edited

Codecov Report

Merging #1413 (284530b) into main (7876cd1) will decrease coverage by 0.0%.
The diff coverage is 88.2%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1413     +/-   ##
=======================================
- Coverage   69.3%   69.2%   -0.1%     
=======================================
  Files        127     127             
  Lines       5496    5475     -21     
=======================================
- Hits        3810    3794     -16     
+ Misses      1538    1534      -4     
+ Partials     148     147      -1
Impacted Files Coverage Δ
.../aws-lambda-go/otellambda/xrayconfig/xrayconfig.go 75.0% <81.8%> (-0.6%) ⬇️
exporters/metric/cortex/cortex.go 73.3% <100.0%> (ø)
exporters/metric/datadog/datadog.go 55.5% <100.0%> (ø)
exporters/metric/dogstatsd/internal/statsd/conn.go 57.5% <100.0%> (ø)

Aneurysm9
Aneurysm9 requested changes Nov 9, 2021
View reviewed changes
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Instrumentation must not depend on an SDK.

@Aneurysm9
Copy link
Member

The failing unit test is a result of no longer trying to yield to the go runtime before invoking ForceFlush(). Once open-telemetry/opentelemetry-go#2335 is available in a release then the test should start passing and this PR can proceed.

@bhautikpip
Copy link
Contributor Author

@MrAlias @dashpole Can we review this change? So that when we release the core this should be ready to merge.

@alolita
Copy link
Member

alolita commented Nov 11, 2021

@open-telemetry/proto-go-approvers @dashpole can you please provide a second review so that this PR can be merged. We have a downstream dependency on this. Many thanks in advance.

dashpole
dashpole approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Just nits. Looks good overall

instrumentation/github.com/aws/aws-lambda-go/otellambda/xrayconfig/xrayconfig.go Outdated
exp, err := otlptracegrpc.New(ctx, otlptracegrpc.WithInsecure())
if err != nil {
return []otellambda.Option{}, err
errorLogger.Println("failed to create exporter: ", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't need to log here. Since we return the error, we expect the caller to handle it (and log it if they would like). Same applies below and elsewhere.

instrumentation/github.com/aws/aws-lambda-go/otellambda/xrayconfig/xrayconfig_test.go Outdated
@@ -154,6 +154,9 @@ func assertSpanEqualsIgnoreTimeAndSpanID(t *testing.T, expected *v1trace.Resourc
func TestWrapEndToEnd(t *testing.T) {
setEnvVars()

ctx := context.Background()
tp, _ := NewTracerProvider(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best to check that the err is non-nil here.

@Aneurysm9 Aneurysm9 merged commit ae5d9e7 into open-telemetry:main Nov 12, 2021
rltoSD referenced this pull request in open-o11y/opentelemetry-go-contrib Dec 21, 2021
@bhautikpip @Aneurysm9 @MrAlias
Go lambda instrumentation enhancements (#1413)
0df2cf8 
* added failure scenario when getting container fails

* fix test case failure

* add changelog

* fix ecs resource detector bug

* fix struct name as per golint suggestion

* minor changes

* added NewResourceDetector func and interface assertions

* fix golint failure

* minor changes to address review comments

* lambda go instrumentation enhancements

* minor changes to address review comments

* update go.mod

* Revert "update go.mod"

This reverts commit 2696b35.

* remove depending on SDK package

* renaming the public API for xrayconfig package

* fix README review suggestions

* remove logger and minor changes

* Update instrumentation/github.com/aws/aws-lambda-go/otellambda/xrayconfig/xrayconfig_test.go

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@dashpole dashpole dashpole approved these changes

@evantorrie evantorrie Awaiting requested review from evantorrie

@jmacd jmacd Awaiting requested review from jmacd

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@MrAlias MrAlias Awaiting requested review from MrAlias

@paivagustavo paivagustavo Awaiting requested review from paivagustavo

@pellared pellared Awaiting requested review from pellared

@XSAM XSAM Awaiting requested review from XSAM

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bhautikpip @Aneurysm9 @alolita @dashpole @MrAlias