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

cleanup connection messages #80

Merged
merged 3 commits into from
Jan 18, 2022
Merged

cleanup connection messages #80

merged 3 commits into from
Jan 18, 2022

Conversation

wasaga
Copy link
Contributor

@wasaga wasaga commented Jan 12, 2022

Summary

cleanup connection state change messages

image

Related issues

Fixes https://github.com/pomerium/internal/issues/715

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@wasaga wasaga added the bug Something isn't working label Jan 12, 2022
Copy link
Contributor

@desimone desimone left a comment

Choose a reason for hiding this comment

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

I like the new error stems better, but could we keep the context as well in the log?

More generally. I think the intent here was to display structured (json) logs, our logging feels like we are going the other direction. From unstructured, forcing structure from error status enums.

@wasaga
Copy link
Contributor Author

wasaga commented Jan 12, 2022

destination address and listen address are available in the same window.
the only other piece that's missing is peer address, which is typically opaque and won't tell people much I believe.

image

Structured logs could be converted into human readable ones. There's no usability improvement in presenting them as JSON.

Copy link

@alexfornuto alexfornuto left a comment

Choose a reason for hiding this comment

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

👍 pending passing tests.

@wasaga wasaga merged commit 71867d1 into master Jan 18, 2022
@wasaga wasaga deleted the wasaga/hide-auth-url branch January 18, 2022 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants