Implement logic for awsxrayproxy extension #4625
Conversation
|
@bhautikpip Please check out, small PR since real logic is already there, just need to initialize |
extension/awsxrayproxy/extension.go
Outdated
| go x.server.ListenAndServe() | ||
| x.logger.Info("X-Ray proxy server started") |
There was a problem hiding this comment.
Perhaps a bit more parity on logging?
Or is this taken care of by extension interface?
|
@Aneurysm9 @mxiamxia If you get a chance would be good to get a review. Thanks |
|
@Aneurysm9 please review |
|
@Aneurysm9 Merged main and build's green again |
anuraaga
force-pushed
the
implement-awsxrayproxy
branch
from
44c7c89
to
942da53
Compare
|
@Aneurysm9 @alolita I understand there's a lot of churn in the repo right now but wondering if it's possible to merge this PR to unblock the launch of xray remote sampling. It's not a big one |
|
I've rebased |
extension/awsxrayproxy/extension.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (x xrayProxy) Shutdown(ctx context.Context) error { | ||
| // TODO(anuraaga): Implement me | ||
| return nil | ||
| return x.server.Close() |
There was a problem hiding this comment.
Can we ensure this honors a timeout if one is set on ctx?
There was a problem hiding this comment.
Switched to shutting down with context
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@anuraaga I think this may need a second look for Anthony's comments |
…tor-contrib into implement-awsxrayproxy
|
@Aneurysm9 Sorry for the delay - I have rebased to latest. |
anuraaga
force-pushed
the
implement-awsxrayproxy
branch
from
ecc1f84
to
aa5e65c
Compare
anuraaga
force-pushed
the
implement-awsxrayproxy
branch
from
aa5e65c
to
05194a5
Compare
|
@Aneurysm9 Can you take another look? Or @dmitryax? I've merged go.mod 10 times or so by now so hoping for an approval before next merge |
|
@Aneurysm9 @dmitryax @alolita Would appreciate another look at this. Thanks |
|
@open-telemetry/collector-contrib-maintainer Any tips on how to move forward on this PR? Presumably I shouldn't need to rebase this every week forever? :P |
|
Waiting for this - centralized sampling config would really help with cost reduction for us. Happy to share usecases if desired |
extension/awsxrayproxy/factory.go
Outdated
| ProxyAddress: "", | ||
| TLSSetting: configtls.TLSClientSetting{ | ||
| Insecure: false, | ||
| ServerName: "", | ||
| }, | ||
| Region: "", | ||
| RoleARN: "", | ||
| AWSEndpoint: "", |
There was a problem hiding this comment.
Are these all the zero value for their type? Do they need to be included explicitly here?
There was a problem hiding this comment.
Thanks, cleaned that up
|
@bogdandrutu @tigrannajaryan This is now approved so should be good to merge. Thanks! |
Implements awsxrayproxy extension by calling into internal/aws/proxy
Update description for Span Processor (open-telemetry#5474) Implement logic for awsxrayproxy extension (open-telemetry#4625) Implements awsxrayproxy extension by calling into internal/aws/proxy Fix main branch, PRs merge race condition (open-telemetry#5506) Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Description:
Implements awsxrayproxy extension by calling into internal/aws/proxy
Link to tracking Issue:
Testing: Unit tests
Documentation: