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

observability naming adjustments #537

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

lucix-aws
Copy link
Contributor

  • Remove dashes from single-package modules smithyoteltracing and smithyotelmetrics. Conventionally, when a module is also meant to be used as a package, it should have a valid package name.
  • align span attribute names with internal recommendations
    • I also removed the api.error_fault attribute since I don't think it really added any value

@lucix-aws lucix-aws requested review from a team as code owners September 18, 2024 19:23

var aerr smithy.APIError
if $errors.As:T(err, &aerr) {
span.SetProperty("error.api.code", aerr.ErrorCode())
span.SetProperty("error.api.message", aerr.ErrorMessage())
span.SetProperty("error.api.fault", aerr.ErrorFault().String())
Copy link
Contributor

Choose a reason for hiding this comment

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

What's fault in the context of Smithy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"client", "server" or "unknown"

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, like "who's fault is this".

I may be biased towards the errors I happen to be looking at right now, but this does seem useful for communicating with customers and saying "this is a client error, not an issue with the server", although I'd defer to your experience if you think this is not important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I'm fine leaving it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re-added

@lucix-aws lucix-aws merged commit 6f58823 into feat-observability Sep 18, 2024
1 check passed
@lucix-aws lucix-aws deleted the feat-observability-modrename branch September 18, 2024 20:06
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