Skip to content
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

✨Adding tracing in fastapi-based services (⚠️ devops) #2558

Merged

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Sep 27, 2021

What do these changes do?

Related issue/s

We are missing a reliable way to monitor REST API calls. As we do have an infrastructure for this given by Jaeger we should use it!

⚠️ devops: add TRACING_THRIFT_COMPACT_ENDPOINT=http://jaeger:5775 in .env to correctly setup tracing

This PR activates tracing in the following services:

  • catalog
  • director-v2
  • datcore-adapter
  • dynamic-sidecar (still needs to be converted to settings-library @GitHK )
  • api-server (still needs to be converted to settings-library @pcrespov )

Bonus:

  • converted catalog to use settings-library
  • setting the hostname in each service by using docker templates. This now is not enabled in the dask-sidecar for a reason that might be discovered later with the sidecar refactoring.

How to test

Checklist

@sanderegg sanderegg added the a:catalog catalog service label Sep 27, 2021
@sanderegg sanderegg added this to the Capra delle nevi milestone Sep 27, 2021
@sanderegg sanderegg self-assigned this Sep 27, 2021
@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #2558 (a57ad4d) into master (a50b617) will decrease coverage by 0.0%.
The diff coverage is 52.4%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2558     +/-   ##
========================================
- Coverage    77.2%   77.1%   -0.1%     
========================================
  Files         613     616      +3     
  Lines       23852   23862     +10     
  Branches     2338    2343      +5     
========================================
- Hits        18423   18413     -10     
- Misses       4801    4821     +20     
  Partials      628     628             
Flag Coverage Δ
integrationtests 65.9% <100.0%> (+<0.1%) ⬆️
unittests 71.6% <52.4%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ibrary/src/models_library/settings/http_clients.py 0.0% <ø> (ø)
.../service-library/src/servicelib/fastapi/tracing.py 0.0% <0.0%> (ø)
.../settings-library/src/settings_library/postgres.py 90.6% <ø> (ø)
...s/catalog/src/simcore_service_catalog/db/events.py 44.8% <0.0%> (+1.4%) ⬆️
...rc/simcore_service_catalog/db/repositories/dags.py 40.5% <ø> (ø)
...rvices/catalog/src/simcore_service_catalog/main.py 0.0% <ø> (ø)
.../simcore_service_catalog/services/access_rights.py 26.5% <ø> (ø)
...c/simcore_service_catalog/core/background_tasks.py 29.7% <4.5%> (-3.6%) ⬇️
...g/src/simcore_service_catalog/services/director.py 41.8% <11.1%> (-1.1%) ⬇️
...catalog/src/simcore_service_catalog/core/events.py 57.5% <18.1%> (-1.5%) ⬇️
... and 13 more

@GitHK
Copy link
Contributor

GitHK commented Sep 29, 2021

@sanderegg servicelib integration for dynamic-sidecar will come in via:

Have a look here, I think this should be enough.

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice! Glad this is coming in.

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice!
Just some minor comments!

@pcrespov pcrespov mentioned this pull request Sep 29, 2021
3 tasks
@pcrespov
Copy link
Member

You might also want to add the CLI? check how i did it in the api-server in PR #2563

@sanderegg sanderegg changed the title ✨Adding tracing in catalog ✨Adding tracing in fastapi-based services Sep 30, 2021
@sanderegg
Copy link
Member Author

@sanderegg servicelib integration for dynamic-sidecar will come in via:

Have a look here, I think this should be enough.

@GitHK : please see this PR concerning api-server. that is what is needed in the dynamic-sidecar.

@sanderegg sanderegg force-pushed the maintenance/active_tracing_fastapi branch 2 times, most recently from 5fb0dac to 0ea9dd1 Compare October 1, 2021 06:38
@sanderegg sanderegg force-pushed the maintenance/active_tracing_fastapi branch 2 times, most recently from bdb4b8c to 374fae4 Compare October 8, 2021 10:16
@sanderegg sanderegg changed the title ✨Adding tracing in fastapi-based services ✨Adding tracing in fastapi-based services (⚠️ devops) Oct 8, 2021
@sanderegg sanderegg force-pushed the maintenance/active_tracing_fastapi branch from 9c5defd to 0278881 Compare October 8, 2021 15:50
@sanderegg sanderegg force-pushed the maintenance/active_tracing_fastapi branch from 0278881 to d3c36e5 Compare October 17, 2021 19:29
@sanderegg sanderegg merged commit 89fef0d into ITISFoundation:master Oct 18, 2021
@sanderegg sanderegg deleted the maintenance/active_tracing_fastapi branch October 18, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:catalog catalog service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants