-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
ci: Setup otel tracing for windows integration tests #46663
Conversation
- run: | | ||
Invoke-WebRequest ` | ||
-Uri "https://github.com/jaegertracing/jaeger/releases/download/v1.50.0/jaeger-1.50.0-windows-amd64.tar.gz" ` | ||
-OutFile ".\jaeger-1.50.0-windows-amd64.tar.gz" | ||
tar -zxvf ".\jaeger-1.50.0-windows-amd64.tar.gz" | ||
Start-Process '.\jaeger-1.50.0-windows-amd64\jaeger-all-in-one.exe' | ||
echo "OTEL_EXPORTER_OTLP_ENDPOINT=http://127.0.0.1:4318" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append | ||
shell: pwsh |
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.
Could be moved to https://github.com/moby/moby/blob/master/.github/actions/setup-tracing/action.yml
Using bash shell with a cond if [ "${{ runner.os }}" = "Windows" ]
.
You should still be able to run jaeger-all-in-one.exe
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.
Oh that's good to know. Unfortunately GH was unhappy with this file and I'm not sure it will work better if I move this step into the setup-tracing
action. See this run: https://github.com/moby/moby/actions/runs/6545687164/job/17775109401#step:3:1. Any ideas?
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.
Unfortunately GH was unhappy with this file and I'm not sure it will work better if I move this step into the
setup-tracing
action. See this run: https://github.com/moby/moby/actions/runs/6545687164/job/17775109401#step:3:1. Any ideas?
Hum maybe the name should be unique? Can you try with name: 'Setup Tracing Windows'
c0546c8
to
289e42a
Compare
289e42a
to
eda88dc
Compare
Hum still this issue:
I wonder if this is because we change the working dir in moby/.github/workflows/.windows.yml Lines 40 to 42 in 70589b8
|
.github/workflows/.windows.yml
Outdated
@@ -271,6 +271,9 @@ jobs: | |||
uses: actions/checkout@v3 | |||
with: | |||
path: ${{ env.GOPATH }}/src/github.com/docker/docker | |||
- | |||
name: Set up tracing | |||
uses: ./.github/actions/setup-tracing |
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.
related to my previous comment, can you try:
uses: ./.github/actions/setup-tracing | |
uses: ./.github/actions/setup-tracing | |
working-directory: ${{ github.workspace }} |
Or maybe absolute:
uses: ./.github/actions/setup-tracing | |
uses: ${{ github.workspace }}/.github/actions/setup-tracing |
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.
5785bed
to
4a6af7e
Compare
4a6af7e
to
c81a417
Compare
Some questions; It looks like we're using a custom action for Linux; is that something that's possible for Windows as well? If that's indeed for the same; I see the action uses a different version of Jaeger ( moby/.github/actions/setup-tracing/action.yml Lines 9 to 11 in 7cabe08
It would be good to user the same version, even if it's not "super" important, so that it's less likely for those versions to be missed if we update. I guess ideally both would be set up using the same action (or something in the same file / dir), but if that's not possible, we should consider adding a comment (when updating thise one, don't forget the other one), or maybe a parameter / env-var for the version. |
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
c81a417
to
5a83beb
Compare
That's what we try to do initially with @crazy-max. We tried both to add a Windows-specific run step to the existing action, and to create a dedicated action. In both cases, this didn't work (see this failure: https://github.com/moby/moby/actions/runs/6545687164/job/17775109401#step:3:1), but we're unable to find the root cause. Hence, we decided to inline the run step in
I'm not a big fan of the env-var as it'd have to be defined either at the repo level (so it'd require admin rights to update it), or in workflows files (so we're back to the duplication problem it tries to avoid). Instead I added a comment in both |
Hm.. right. Yeah, not ideal, but I guess "it is what it is". Perhaps we can try again in future
Yeah, I was considering that we could have an easy-to-grep A comment is fine for now. |
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.
LGTM
Related to:
- What I did
Note that for both Linux and Windows builds, the trace file can't be fully imported in Jaeger due to its size. In addition, big trace files also tend to create many many DOM nodes, leading to unresponsive browser tab. One have to tweak the CI config on a given PR to limit how much integration tests are run to get a usable trace file.
- A picture of a cute animal (not mandatory but encouraged)