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

Print DNS errors #1607

Merged
merged 2 commits into from
Jun 27, 2016
Merged

Print DNS errors #1607

merged 2 commits into from
Jun 27, 2016

Conversation

2opremio
Copy link
Contributor

Fixes #1577

@tomwilkie
Copy link
Contributor

By default we try and resolve scope.weave.local, which will fail if people aren't running weave (most aren't), so this will pollute the logs, no?

@2opremio
Copy link
Contributor Author

2opremio commented Jun 23, 2016

By default we try and resolve scope.weave.local, which will fail if people aren't running weave (most aren't), so this will pollute the logs, no?

True, it's similar to #1599 (comment) but I don't think the solution is to remove all errors, including legitimate ones. In fact, in the example I just mentioned we do the exact opposite.

How about only including the scope.weave.local target once weave is found?

@tomwilkie
Copy link
Contributor

How about only including the scope.weave.local target once weave is found?

I don't like the hardcoding - how about we only report resolution errors once (or n times) per target and silence them after? And similarly, when the start succeeding again, we log something.

@2opremio
Copy link
Contributor Author

I don't like the hardcoding

It's not hardcoding, I wrote scope.weave.local to continue the conversation but we will use whatever is provided as --probe.weave.hostname

It makes sense, if weave is not running, why are we even targeting that host?

how about we only report resolution errors once (or n times) per target and silence them after? And similarly, when the start succeeding again, we log something.

That's an option too, but I prefer the former because it won't display irrelevant errors

@tomwilkie
Copy link
Contributor

It makes sense, if weave is not running, why are we even targeting that host?

We don't know if we are running weave or not. We assume we are, and try to use it.

I think you're suggesting we only log errors after we have successfully resolved a host? But you are also arguing we should show resolution errors? Or would you just do this as a special case for resolution of scope's weavedns hostname?

I think it too special cased, and a more general rule (like only logging the first error) would be easier to understand.

@2opremio
Copy link
Contributor Author

2opremio commented Jun 27, 2016

We don't know if we are running weave or not. We assume we are, and try to use it.

I think you're suggesting we only log errors after we have successfully resolved a host? But you are also arguing we should show resolution errors? Or would you just do this as a special case for resolution of scope's weavedns hostname?

What I am suggesting is we only register the weave target in the probes after we have proof of weave working (e.g. after weave ps succeeds or the weave status request suceeds ...)

I think it too special cased, and a more general rule (like only logging the first error) would be easier to understand.

We can do both. Only log the first error and only add the WeaveDNS target once we know weave is successfully running.

@tomwilkie
Copy link
Contributor

LGTM; I see we only get/set from the single goroutine - does it matter that we create the map on a different one?

@2opremio
Copy link
Contributor Author

does it matter that we create the map on a different one?

Not AFAIK

@2opremio 2opremio merged commit 05e6193 into master Jun 27, 2016
@2opremio 2opremio deleted the 1577-print-dns-errors branch June 27, 2016 23:58
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