Add AppRunner default config for X-Ray integration #647
Conversation
Codecov Report
@@ Coverage Diff @@
## main #647 +/- ##
==========================================
- Coverage 64.54% 64.38% -0.17%
==========================================
Files 8 8
Lines 220 219 -1
==========================================
- Hits 142 141 -1
Misses 62 62
Partials 16 16
Continue to review full report at Codecov.
|
cmd/awscollector/Dockerfile
Outdated
| @@ -52,6 +52,7 @@ COPY --from=build /workspace/config/eks/prometheus/config-all.yaml /etc/eks/prom | |||
| COPY --from=build /workspace/config/ecs/container-insights/otel-task-metrics-config.yaml /etc/ecs/container-insights/otel-task-metrics-config.yaml | |||
| COPY --from=build /workspace/config/ecs/ecs-default-config.yaml /etc/ecs/ecs-default-config.yaml | |||
| COPY --from=build /workspace/config/ecs/otel-instance-metrics-config.yaml /etc/ecs/otel-instance-metrics-config.yaml | |||
| COPY --from=build /workspace/config/apprunner/apprunner-xray-config.yaml /etc/ecs/apprunner-xray-config.yaml | |||
There was a problem hiding this comment.
Recommend to rephrase. /etc/ecs/apprunner-xray-config.yaml to /etc/apprunner/apprunner-xray-config.yaml
| limit_percentage: 40 # 40% of total allocated memory | ||
| spike_limit_percentage: 8 # 100 * 40% * 20% = 8%: 20 percent from the total limited memory |
There was a problem hiding this comment.
ECS Task total allocated Mem: 1000MB Max
limit_percent(hard): 40% = 400MB - hard limit threshold
spike limit(soft): 8% = 1000MB * 40% * 20% = 320MB - soft limit threshold
Both will drop data. Hard limit will perform GCs more frequently
There was a problem hiding this comment.
Reviewed default-config with our Product Manager Akshay and got sign-off on 10/4. 40% memory limit percentage works for him based on today’s App Runner resource quota and adot known performance cost.
@mxiamxia Please kindly let us know what’s the next step for this config setup from ADOT side.
cmd/awscollector/Dockerfile
Outdated
| @@ -52,6 +52,7 @@ COPY --from=build /workspace/config/eks/prometheus/config-all.yaml /etc/eks/prom | |||
| COPY --from=build /workspace/config/ecs/container-insights/otel-task-metrics-config.yaml /etc/ecs/container-insights/otel-task-metrics-config.yaml | |||
| COPY --from=build /workspace/config/ecs/ecs-default-config.yaml /etc/ecs/ecs-default-config.yaml | |||
| COPY --from=build /workspace/config/ecs/otel-instance-metrics-config.yaml /etc/ecs/otel-instance-metrics-config.yaml | |||
| COPY --from=build /workspace/config/apprunner/apprunner-xray-config.yaml /etc/apprunner/apprunner-xray-config.yaml | |||
There was a problem hiding this comment.
Nit: this is just for my knowledge, trying to make the config generic. should we call it apprunner-default-config.yaml instead or apprunner-otel-config.yaml since in future apprunner will support not only xray as trace destination.
There was a problem hiding this comment.
I agree it is a valid point. changed.
| grpc: | ||
| endpoint: 0.0.0.0:4317 | ||
| http: | ||
| endpoint: 0.0.0.0:55681 |
There was a problem hiding this comment.
If this feature hasn't been released yet, is there any way we can use the official 4318 port for this instead of this legacy one?
There was a problem hiding this comment.
do you know if OTel SDK has updated its default HTTP port to 4318 yet? I previously only updated OTel SDK for gRPC on new port. Thanks
There was a problem hiding this comment.
I'm not sure across all the languages, java uses it as default as of release coming out this week.
Perhaps this needs to be configured for both 4318 and 55681 temporarily to handle older and newer SDKs.
There was a problem hiding this comment.
@mxiamxia Do you think you could add both ports? So we can get this closed out, and handle the remaining tasks ourselves.
There was a problem hiding this comment.
Is app runner a sidecar deployment? The default without specifying an endpoint is to listen on localhost on old and new ports. For sidecar we shouldn't define an endpoint. Otherwise we should probably set two receivers
If min doesn't get back I can make the change Monday if I know that.
There was a problem hiding this comment.
It will be a sidecar deployment of the collector.
For sidecar we shouldn't define an endpoint.
Why is this?
There was a problem hiding this comment.
I think we can remove these defined (default) endpoints and have the collector pickup it's default endpoints. Collector will support the new port and legacy ports in the receiver.
https://github.com/open-telemetry/opentelemetry-collector/blob/main/receiver/otlpreceiver/otlp.go#L146
Yes, Yiming has tested and approved the PR. |
Description: Describe what has changed.
Add AppRunner default config for X-Ray integration