feat: report integration errors #187
Conversation
dmehala
force-pushed
the
dmehala/send-integration-errors
branch
from
3b9d7d1 to
eeeef60
Compare
|
On amazonlinux:2.0.20230418.0 there's a failure: AssertionError: 1 != 0 : ['nginx: [emerg] dlopen() "/datadog-tests/ngx_http_datadog_module.so" failed (/datadog-tests/ngx_http_datadog_module.so: undefined symbol: memfd_create) in /tmp/fastcgi_auto.conf:5', 'nginx: configuration file /tmp/fastcgi_auto.conf test failed', ''] memfd_create was added in glibc 2.27 (and linux 3.17), but that image uses 2.26... You'll need to either provide the function (using |
|
Thank you @cataphract , I'll need to provide the function because it's part of a new feature relying on |
| @@ -53,7 +54,8 @@ ngx_int_t on_enter_block(ngx_http_request_t *request) noexcept try { | |||
| } else { | |||
| try { | |||
| context->on_change_block(request, core_loc_conf, loc_conf); | |||
| } catch (...) { | |||
| } catch (const std::exception &e) { | |||
| telemetry::report_error_log(e.what(), CURRENT_FRAME(request)); | |||
There was a problem hiding this comment.
Using directly e.what() are we sure we are not taking any risks or leaking PII ?
There was a problem hiding this comment.
In the entire codebase, exceptions can only be thrown by our integrations or dependencies we use. Unless we throw exceptions with PIIs, which we don't at the moment, I believe we are ok. Follow up question, how do you handle PIIs for other languages?
There was a problem hiding this comment.
PHP is in the same case as you so we are not handling them (and I will have to check this is safe).
In python i'm ensuring the log message is constant.
If you know that we are not throwing exceptions with PIIs we don't have to make any changes.
src/telemetry_util.h
Outdated
| std::string_view file, size_t line, | ||
| std::string_view function) { | ||
| // clang-format off | ||
| return std::format("Exception catched:\n" |
There was a problem hiding this comment.
| return std::format("Exception catched:\n" | |
| return std::format("Exception caught:\n" |
There was a problem hiding this comment.
That's an interesting one. catched is actually valid English and commonly used before 19th century, which is when caught started to replaced it.
There was a problem hiding this comment.
Oh, so every time I said catched I was not making any mistake. Nice to know haha
| constexpr std::string_view prefix = "nginx-datadog"; | ||
| size_t pos = absolute_filepath.find(prefix); | ||
| return pos != std::string_view::npos ? absolute_filepath.substr(pos) | ||
| : absolute_filepath; |
There was a problem hiding this comment.
The problem with this method is, if nginx-datadog is not in the string (which I agree with you should not happen), we will leak the full_path, can we add a safe guard please ?
There was a problem hiding this comment.
Could you clarify what the potential risks are of exposing the full path? (which is highly improbable unless the repo change name or the ci building the project is cloning the repo under another name). Also, could you elaborate on what you mean by safeguard? Do you have any suggestions or proposals in mind?
There was a problem hiding this comment.
For instance, i have my name.surname in my full_path, which is PII. In PHP, I did this:
https://github.com/DataDog/dd-trace-php/blob/0e702530af821f358ccd1dbdecd9535728eed459/ext/telemetry.c#L50
I agree that is a bit overkill and I could accept to not include that but it does not add runtime overhead so I think "prevention is better than cure" is the way to go.
There was a problem hiding this comment.
Interesting 🤔 Make sense. I'll adjust the method to return only the name of the file instead of the full path. Thanks for raising your concerns.
Co-authored-by: Louis Tricot <[email protected]>
Resolves AIDM-394