Skip to content

Conversation

@r1viollet
Copy link
Collaborator

What does this PR do?

Introduce a helper to cleanup the libddprof errors

Motivation

It is easy to make mistakes when taking into account libddprof errors.

Introduce a helper to cleanup the libddprof errors

inline void log_warn_and_drop_error(std::string_view message,
ddog_Error *error) {
defer { ddog_Error_drop(error); };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use defer here ?


LG_ERR("[EXPORTER] Failure generate tag (%.*s)",
(int)push_tag_res.err.message.len, push_tag_res.err.message.ptr);
log_warn_and_drop_error("[EXPORTER] Failure generate tag",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't the error message be logged twice ?
What is the benefit of using log_warn_and_drop_error ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I was not using ddog_Error_message. The aim is to have a function you can always call in case of error and not think too hard about what to do.
Ideally it would handle the error returns with it, though that behavior is different across the files.

exporter->_exporter = res_exporter.ok;
} else {
defer { ddog_Error_drop(&res_exporter.err); };
log_warn_and_drop_error("[EXPORTER] Failure form ddog_prof_Exporter_new",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

double log to be fixed

@r1viollet
Copy link
Collaborator Author

Possible improvements:

  • A C++ type could wrap and drop resources.

@r1viollet r1viollet marked this pull request as draft May 3, 2023 09:44
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.

3 participants