-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add optional origin field #171
Conversation
iamsouravin
commented
May 28, 2020
- Update license header.
- Add RATIONALE.md.
- Add encoding for optional origin field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a setting in the wrong place. The collector (thing running storage) won't have the valid details for the app unless it is running on the same host. Running on the same host is an assumption. It seems more useful to add tags for this when recording the data, then re-map them to fields as necessary.
For example, Brave can record this as a constant as could any instrumentation, especially with a good default. It seems that the env variable listed here is not an AWS env variable so it won't likely be correct either. Maybe we should step back and look at how other libraries properly populate this?
from the docs https://docs.aws.amazon.com/xray/latest/devguide/xray-api-segmentdocuments.html:
origin – The type of AWS resource running your application.
Supported Values
AWS::EC2::Instance – An Amazon EC2 instance.
AWS::ECS::Container – An Amazon ECS container.
AWS::ElasticBeanstalk::Environment – An Elastic Beanstalk environment.
When multiple values are applicable to your application, use the one that is most specific. For example, a Multicontainer Docker Elastic Beanstalk environment runs your application on an Amazon ECS container, which in turn runs on an Amazon EC2 instance. In this case you would set the origin to AWS::ElasticBeanstalk::Environment as the environment is the parent of the other two resources.
storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPMessageEncoder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks nearly there!
storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPMessageEncoder.java
Outdated
Show resolved
Hide resolved
storage-xray-udp/src/test/java/zipkin2/storage/xray_udp/UDPMessageEncoderTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping with this!
One high level note is origin can actually be inferred by other tags, if we were to actually populate them.
The segment has an aws
struct here
So instead of origin
, we could process tags like this into the segment for a more complete picture (and origin could be inferred)
aws.beanstalk.environment_name
aws.beanstalk.version_label
aws.beanstalk.deployment_id
aws.ecs.container
aws.ec2.instance_id
aws.ec2.availability_zone
What do you think?
storage-xray-udp/src/main/java/zipkin2/storage/xray_udp/UDPMessageEncoder.java
Outdated
Show resolved
Hide resolved
Thanks @adriancole and @anuraaga for the review comments. Incorporated the suggested changes. Deferring the auto-detection piece for a later thread as discussed with @anuraaga since it involves a larger discussion with the general community integration with other projects like OpenTelemetry. |
I accidentally closed this somehow and sadly can't re-open it. I will cherry-pick the changes in! |
200206f and opened #172 for follow-up. thanks @iamsouravin and @anuraaga |