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

error tags #244

Merged
merged 4 commits into from
Sep 20, 2022
Merged

error tags #244

merged 4 commits into from
Sep 20, 2022

Conversation

dgoffredo
Copy link
Contributor

@dgoffredo dgoffredo commented Sep 13, 2022

This revision makes spans aware of the error.msg, error.stack, and error.type tags.

These changes grew out of Andrew Glaude's investigation of compatibility between the C++ tracer and "error tracking": https://docs.datadoghq.com/tracing/error_tracking/.

The new behavior is best summarized by the table of test cases in the added SECTION of test/span_test.cpp.

@ajgajg1134 (he's on vacation) FYI.

Also, when these changes are finalized, we'll want to update these docs: https://docs.datadoghq.com/tracing/trace_collection/custom_instrumentation/cpp/#set-errors-on-a-span

@dgoffredo dgoffredo requested a review from cgilmour September 13, 2022 16:45
Copy link
Contributor

@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.

Does this still apply?

    // Errors can be a flag or a detailed message.
    // Empty or false-y values indicate no error.
    // Any other value will mark the span to indicate an error occured.

When it's neither falsey or truthy, does it make sense to assign the error tag's contents to error.msg?

@dgoffredo
Copy link
Contributor Author

The code comment is still true, yes. It would be more true if we set error.msg.

I considered adding the "if neither truthy nor falsy, and not empty, then populate error.msg instead of error" logic. My take is that it's a little harder to remember and explain that policy.

The benefit of the idea, though, is that it means the error tag would only ever be one of these values, which is kind of nice. In all other cases either there would be no error-related tags, or a more descriptive tag would be set instead.

Alright, I'll try it.

@dgoffredo
Copy link
Contributor Author

Another thought: If someone sets the error tag to an error code value, e.g. errno, then today it works as they might expect: zero means "no error," and other values mean "this is the error code." Moving values like "-2" or "4" to error.msg would mess with that (if that ever actually happens).

@dgoffredo
Copy link
Contributor Author

Eh, for that we could instead encourage an error.code. The thing to address here is that Datadog thinks of error as a namespace of tags, not as a tag itself. Ok.

Copy link
Contributor

@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.

Might need refactoring at some point, but no real objection about it functionally

namespace datadog {
namespace opentracing {

bool stob(const std::string& str, bool fallback);
bool isbool(const std::string& str);

enum class Tribool { False, True, Neither };
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge fan of this, but also didn't like the other parts (that I wrote) for handling/parsing boolean values.
And its usage is isolated-enough to not be particularly concerned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last chance for name changes. Ideas: enum class Trigun, enum class Tribble, enum class Tribulation, enum class Dribble, enum class Boolble.

@dgoffredo dgoffredo merged commit 7968076 into master Sep 20, 2022
@dgoffredo dgoffredo deleted the david.goffredo/error-tags branch September 20, 2022 22:09
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.

2 participants