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

Add uWSGI support #41

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Add uWSGI support #41

merged 1 commit into from
Sep 14, 2023

Conversation

dmehala
Copy link
Contributor

@dmehala dmehala commented Sep 7, 2023

This PR add support for uWSGI. When uwsgi_pass directives are encountered, the module automatically propagate traces.

Content:

  • Add uWSGI service for testing and as an example
  • Add propagation tests for uwsgi directives
  • Update example/README.md
  • Ignore *.pyc

Resolves #38

Copy link

@cgilmour cgilmour left a comment

Choose a reason for hiding this comment

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

Looks really good. I've put some comments below to consider before approving.

@@ -12,6 +12,7 @@ services:
- `http` runs a NodeJS HTTP server that listens on port 8080.
- `fastcgi` runs a NodeJS FastCGI server that listens on port 8080.
- `grpc` runs a NodeJS gRPC server that listens on port 1337.
- `uwsgi` runs a uWSGI server using the `uwsgi` protocol and listens on port 8080.
Copy link

Choose a reason for hiding this comment

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

For consistency, this could mention Python the way others have mentioned NodeJS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in d475f16

Copy link

Choose a reason for hiding this comment

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

Was this added intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all. I guess it was before I added the new rule in .gitignore. Resolved in d475f16

;; ddtrace required options
enable-threads = 1
lazy-apps = 1
import=ddtrace.bootstrap.sitecustomize
Copy link

Choose a reason for hiding this comment

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

It took me a couple of minutes to figure this out, because the spot where ddtrace is installed is quite subtle.
Maybe some additional comments would be helpful

Copy link
Contributor Author

@dmehala dmehala Sep 8, 2023

Choose a reason for hiding this comment

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

I added more informations. As a non native english speaker, any suggestion will be appreciated :) Resolved in d475f16

const auto guard = defer([&]() { cf->args = old_args; });

for (const std::string_view key : keys) {
// NOTE(@dmehala): uWSGI uses the same key header convention thant fastcgi
Copy link

Choose a reason for hiding this comment

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

thant? Maybe as?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brainfart 🤡 Thank you! Resolved in d475f16


return static_cast<char *>(NGX_CONF_OK);
} catch (const std::exception &e) {
ngx_log_error(NGX_LOG_ERR, cf->log, 0, "datadog_grpc_propagate_context failed: %s", e.what());
Copy link

Choose a reason for hiding this comment

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

grpc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed in d475f16

@dgoffredo dgoffredo mentioned this pull request Sep 12, 2023
Copy link
Contributor

@dgoffredo dgoffredo left a comment

Choose a reason for hiding this comment

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

Perfect.

Please see #42 for some documentation tweaks.

dgoffredo added a commit that referenced this pull request Sep 14, 2023
* __pycache__ directory. I know, I know...

* original wording was fine, but I prefer this

* more opinions on comment wording

* mention uwsgi_pass in the docs for datadog_proxy_directive
- Add uWSGI service for testing and as an example
- Add propagation tests for uwsgi directives
- Update example/README.md
- Ignore *.pyc
- Mention uwsgi_pass in the docs for datadog_proxy_directive

Resolves #38
@dmehala dmehala merged commit 582d1cc into master Sep 14, 2023
8 checks passed
@dmehala dmehala deleted the dmehala/uwsgi branch September 14, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Support automatically configuring uWSGI pass
3 participants