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

feat(error): show internal correlation id in error messages #411

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

apricote
Copy link
Member

@apricote apricote commented Apr 19, 2024

This commit changes the way error responses from the API are formatted for display to users. When available, it adds the Correlation (Trace) ID header to the error string.

We often have users that post the error string, but rarely do users have debug logging active. By adding the correlation ID to error messages, we can more quickly investigate why something went wrong using the Hetzner internal tracing system.

This is common practice in Web Apps, they usually show a request/trace ID or an encrypted blob in case of an internal server error.

@apricote apricote added Improvement backport release-1.x Open PR against release-1.x with these changes after the PR is merged. labels Apr 19, 2024
@apricote apricote self-assigned this Apr 19, 2024
@apricote apricote requested a review from a team as a code owner April 19, 2024 07:29
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.43%. Comparing base (d4f67cb) to head (12af0ca).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #411      +/-   ##
==========================================
+ Coverage   80.41%   80.43%   +0.02%     
==========================================
  Files          32       32              
  Lines        5750     5756       +6     
==========================================
+ Hits         4624     4630       +6     
  Misses        718      718              
  Partials      408      408              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hcloud/client.go Outdated
Comment on lines 417 to 424
// InternalCorrelationID returns the unique ID of the request as set by the API. This ID can help with support requests,
// as it allows the people working on identify this request in particular.
func (r *Response) InternalCorrelationID() string {
return r.Header.Get(headerCorrelationID)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if we should add this to our public interface. There is no guarantee that this header will always be exposed in the API. I am fine with removing this from the public interface.

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep it private for now, until we have a better vision on how we want to do tracing in our integrations.

I am not sure if we have plans for extending the reach of our tracing capabilities, but I think it would be awesome to not stop tracing at the boundaries of the public API, but try to push it all the way in our integrations' logic.

Maybe we are allowed to generate this correlation ID, and let the API reuse it or wrap it. I'd love to have the traces that start from a hccm call and come all the way back :).

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think keeping it private is better. If someone needed the correlation ID, it would be accessible over the Headers field anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to internalCorrelationID (private)

Copy link
Member

@jooola jooola left a comment

Choose a reason for hiding this comment

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

I think it's a great addition! I'll implement it in python once we agree on how we should handle this!

hcloud/error.go Outdated Show resolved Hide resolved
hcloud/error.go Outdated Show resolved Hide resolved
hcloud/client.go Outdated
Comment on lines 417 to 424
// InternalCorrelationID returns the unique ID of the request as set by the API. This ID can help with support requests,
// as it allows the people working on identify this request in particular.
func (r *Response) InternalCorrelationID() string {
return r.Header.Get(headerCorrelationID)
}

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep it private for now, until we have a better vision on how we want to do tracing in our integrations.

I am not sure if we have plans for extending the reach of our tracing capabilities, but I think it would be awesome to not stop tracing at the boundaries of the public API, but try to push it all the way in our integrations' logic.

Maybe we are allowed to generate this correlation ID, and let the API reuse it or wrap it. I'd love to have the traces that start from a hccm call and come all the way back :).

This commit changes the way error responses from the API are formatted
for display to users. When available, it adds the Correlation (Trace) ID
header to the error string.

We often have users that post the error string, but rarely do users
have debug logging active. By adding the correlation ID to error messages,
we can more quickly investigate why something went wrong using the Hetzner
internal tracing system.

This is common practice in Web Apps, they usually show a request/trace
ID or an encrypted blob in case of an internal server errror.
@apricote apricote changed the title feat(error): show internal correlation id for server errors (5xx) feat(error): show internal correlation id in error messages Apr 22, 2024
@apricote apricote requested review from phm07 and jooola April 22, 2024 08:55
@apricote apricote merged commit 6c96d19 into main Apr 22, 2024
5 checks passed
@apricote apricote deleted the error-correlation-id branch April 22, 2024 09:09
github-actions bot pushed a commit that referenced this pull request Apr 22, 2024
This commit changes the way error responses from the API are formatted
for display to users. When available, it adds the Correlation (Trace) ID
header to the error string.

We often have users that post the error string, but rarely do users have
debug logging active. By adding the correlation ID to error messages, we
can more quickly investigate why something went wrong using the Hetzner
internal tracing system.

This is common practice in Web Apps, they usually show a request/trace
ID or an encrypted blob in case of an internal server error.

(cherry picked from commit 6c96d19)
apricote added a commit that referenced this pull request Apr 22, 2024
This commit changes the way error responses from the API are formatted for display to users. When available, it adds the Correlation (Trace) ID header to the error string.

We often have users that post the error string, but rarely do users have debug logging active. By adding the correlation ID to error messages, we can more quickly investigate why something went wrong using the Hetzner internal tracing system.

This is common practice in Web Apps, they usually show a request/trace ID or an encrypted blob in case of an internal server error.

Backport 6c96d19 from #411.

Co-authored-by: Julian Tölle <julian.toelle@hetzner-cloud.de>
jooola pushed a commit that referenced this pull request May 6, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.8.0](v2.7.2...v2.8.0)
(2024-05-06)


### Features

* **error:** show internal correlation id in error messages
([#411](#411))
([6c96d19](6c96d19))
* implement actions waiter
([#407](#407))
([1e3fa70](1e3fa70))
* require Go &gt;= 1.21
([#424](#424))
([d4f4000](d4f4000))


### Bug Fixes

* improve error message format with correlation id
([#430](#430))
([013477f](013477f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-1.x Open PR against release-1.x with these changes after the PR is merged. Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants