Handle sigterm better in instrumentation - #16631
Conversation
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
🚀 Image pushed to AWS ECRImage digest: 🐛 Run the image locallyThe following can be used to run the image locally: # Refer to image using the immutable digest. Find alternatives below.
IMAGE_IDENTIFIER="@sha256:ff635656400e162bdf373b065d6202b75be18cd027e271d0ceb940e31100b0d5"
# Refer to image using branch tag
# IMAGE_IDENTIFIER=":branch-ac-jr-handle-sigterm-better-in-instrumentation"
# Refer to image using build tag
# IMAGE_IDENTIFIER=":build-30721"
# Refer to image via the GitHub commit SHA tag
# IMAGE_IDENTIFIER=":sha-b30ca953d77a6fe1e10c02e34c5519cf29ba34ef"
# Set environment variables for the AWS CLI
AWS_PROFILE="<A_PROFILE_FROM_JANUS>"
AWS_DEFAULT_REGION="eu-west-1"
IMAGE_ACCOUNT_ID=$(aws ssm get-parameter --name /organisation/accounts/deployTools --query "Parameter.Value" --output text)
REGISTRY="${IMAGE_ACCOUNT_ID}.dkr.ecr.${AWS_DEFAULT_REGION}.amazonaws.com"
IMAGE="${REGISTRY}/guardian/dotcom-rendering${IMAGE_IDENTIFIER}"
# Login to AWS ECR https://docs.aws.amazon.com/AmazonECR/latest/userguide/registry_auth.html
aws ecr get-login-password | docker login --username AWS --password-stdin $REGISTRY
# Pull the image
docker pull $IMAGE
# Run the image. You'll likely need to set additional flags. See https://docs.docker.com/reference/cli/docker/container/run.
docker run $IMAGE |
7ff26c7 to
d21039b
Compare
github-actions
Bot
removed
the
run_chromatic
|
|
||
| // Instrumentation is preloaded into the same process as the app server. | ||
| // It should react to process SIGTERM, but not own process/server lifecycle. | ||
| let shutdownPromise: Promise<void> | null = null; |
There was a problem hiding this comment.
I'd consider null to be correct here. It's not that it's never been defined, it's that it is currently explicitly not (yet) set.
null is more like None.
| If fetched using `aws xray batch-get-traces`, the trace document will contain a `Segment` field, which contains a | ||
| `Document` field. The `Document` field is a JSON string containing the trace data we want. | ||
|
|
||
| An example CLI request to view a single trace document as json is: |
There was a problem hiding this comment.
minor: Would the DX of using xray improve if we add it to Grafana?
There was a problem hiding this comment.
Yes - but that should be a job for another ticket, and might be worth discussing for cost reasons. There's a lot of data.
| "throttle": false, | ||
| "http": { | ||
| "request": { | ||
| "url": "http://tag-page-rendering-jr.local.dev-gutools.co.uk/Article", |
There was a problem hiding this comment.
It's not supposed to be prod or code data. It's representative.
| return shutdownPromise; | ||
| }; | ||
|
|
||
| process.on('SIGTERM', () => { |
There was a problem hiding this comment.
I could use more context to be able to review this change. Can you share a bit more about the thinking behind it? Is it a response to #16595 (comment) ?
A bit a context from my side. Originally I didn't add this SIGTERM handling as it's not in these docs https://opentelemetry.io/docs/languages/js/getting-started/nodejs/. I ended up adding it because I found it in https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-sdk-node/README.md . Truth be told I didn't think about it with much depth!
There was a problem hiding this comment.
It is in response to that comment. Then a bit of a discuss.
The instrumentation should take care of closing down the instrumentation. It would be a behavioural change for it to terminate the whole process. I'm not sure if anything in the server responds to sigterm at all, but whatever we currently do should not change here.
3 participants
What does this change?
First iteration on the original instrumentation is to handle SIGTERM in the instrumentation without closing the process.
Why?
The instrumentation should not be making actions which affect the main process.
How has this change been tested?
Screenshots