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

telemetry: try guess hostname or external IP addr for metrics #2412

Merged
merged 5 commits into from
Aug 3, 2021

Conversation

wasaga
Copy link
Contributor

@wasaga wasaga commented Jul 30, 2021

Summary

Try guess hostname or external IP addr for metrics.

Related issues

Fixes https://github.com/pomerium/pomerium-console/issues/1490

Checklist

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

@wasaga wasaga self-assigned this Jul 30, 2021
@wasaga wasaga requested a review from a team as a code owner July 30, 2021 23:16
@codeclimate
Copy link

codeclimate bot commented Jul 30, 2021

Code Climate has analyzed commit d6d8e41 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Contributor

@calebdoxsey calebdoxsey left a comment

Choose a reason for hiding this comment

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

Trying to guess a FQDN seems like a hopeless task.

If we really wanted to fix this we could use the pion library with ICE. But I still think the whole telemetry stack is way out of scope, so wouldn't advocate building our own UDP service mesh to get it to work.

I guess if we're ok with a half broken solution this is fine.

@travisgroth
Copy link
Contributor

We will discuss on monday.

@travisgroth
Copy link
Contributor

It's understood that fqdn detection isn't totally reliable; the desire is to have telemetry work out of the box for users if/when possible. We will likely get feedback on common scenarios where it doesn't work and can fine tune from there.

It does look like there's an issue with tests currently, though.

@coveralls
Copy link

coveralls commented Aug 3, 2021

Coverage Status

Coverage decreased (-0.2%) to 65.077% when pulling d6d8e41 on wasaga/telemetry-try-guess-hostname into 94eb3c1 on master.

@wasaga wasaga merged commit 204aa30 into master Aug 3, 2021
@wasaga wasaga deleted the wasaga/telemetry-try-guess-hostname branch August 3, 2021 22:10
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.

None yet

4 participants