potiuk commented on a change in pull request #20847:
URL: https://github.com/apache/airflow/pull/20847#discussion_r783731322
##########
File path: scripts/ci/docker-compose/base.yml
##########
@@ -78,16 +78,35 @@ services:
- VERSION_SUFFIX_FOR_PYPI=${VERSION_SUFFIX_FOR_PYPI}
- VERSION_SUFFIX_FOR_SVN=${VERSION_SUFFIX_FOR_SVN}
- WHEEL_VERSION=${WHEEL_VERSION}
+ - OTEL_TRACES_EXPORTER=jaeger_thrift
+ - OTEL_SERVICE_NAME=airflow-service
+ - OTEL_EXPORTER_JAEGER_ENDPOINT=http://jaeger:14268/api/traces
+
volumes:
# Pass docker to inside of the container so that Kind and Moto tests can
use it.
- /var/run/docker.sock:/var/run/docker.sock
- - /dev/urandom:/dev/random # Required to get non-blocking entropy
source
+ - /dev/urandom:/dev/random # Required to get non-blocking entropy source
ports:
- "${SSH_PORT}:22"
- "${WEBSERVER_HOST_PORT}:8080"
- "${FLOWER_HOST_PORT}:5555"
cap_add:
- SYS_PTRACE
+ depends_on:
Review comment:
This whole part shoudl be separated from the base.yaml.
What we really want to achieve here is to only start jaegger and enable the
open-telemetry instrumentation when "jaegger" integration is enabled. In
Breeze this is done with optional `--integration` flag.
Eventually we want to have the jaegger integration only enabled when you run
airlfow with this command:
```
./breeze start-airflow --integration jaegger
```
You might want to take a look at the way how some other integrations are
done - for example "mongodb' and do it simlarly. There are few places that need
to be modified:
* Docker compose file for integration:
https://github.com/apache/airflow/blob/main/scripts/ci/docker-compose/integration-mongo.yml
- you should have similar `integration-jaegger.yml' where all things that you
added to base.yaml are moved. The `--integration jaegger` flag will
automatically "merge" the `integration-jaegger.yml` when docker-compose is run.
* Integration check when starting airflow:
https://github.com/apache/airflow/blob/c49d6ec8b67e48d0c0fba1fe30c00f590f88ae65/scripts/in_container/check_environment.sh#L164
- the `check_integration` function should check if jaegger is started (you
should be able to connect to jaegger:16686 port I believe.
* Breeze autocomplete should include the integration:
https://github.com/apache/airflow/blob/c49d6ec8b67e48d0c0fba1fe30c00f590f88ae65/breeze-complete#L28
(note that you will have to do `. ./breeze-complete` whenever you modify
that file to get the changes effective for autocompletion
I think that is it - pre-commits shoudl automatically update all other
relevant places when you add new integration and
it should work out-of-the-box (if you have troubles with it - ping me - I
might not be super-responsive next few days but I will respond eventually)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
