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

adding probe error log support #383

Closed
wants to merge 2 commits into from

Conversation

QingsongYao
Copy link

this PR address issue #350 due to we need to keep failed probe history longer for people to investigate issues. debug logging can not address this issue due to people should not need to look at log to troubleshot probe issue. when probing large number of sites, the failed log entry will be override quickly by succeed logs, so we create a separate entry for this.

I have no tested the change yet due to don't know how to setup build and test on local env

@QingsongYao QingsongYao force-pushed the master branch 7 times, most recently from 39fdd69 to f3fe96e Compare November 16, 2018 06:20
@brian-brazil
Copy link
Contributor

I think this would be better combined with the existing list, and reusing the existing flag.

@QingsongYao
Copy link
Author

It make sense that existing list track succeed requests, new failed requested list tracked failed requests. I will reusing the existing flag

@brian-brazil
Copy link
Contributor

The existing list tracks all requests.

@QingsongYao
Copy link
Author

The failed request log will be phased out quickly if we using the same list.

@brian-brazil
Copy link
Contributor

You can have two lists, and merge them.

@QingsongYao
Copy link
Author

Thank you for the suggestion. I made change to this PR to use two lists and merged them when display to UI.

@@ -285,14 +290,18 @@ func main() {
<h2>Recent Probes</h2>
<table border='1'><tr><th>Module</th><th>Target</th><th>Result</th><th>Debug</th>`))

results := rh.List()
results := eh.List()
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not merging the lists, it should be ordered by time.

Copy link
Author

Choose a reason for hiding this comment

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

result struct has no datetime field, so it will be hard to merge and sort by time.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the ids

Copy link
Author

Choose a reason for hiding this comment

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

The ids in two list might conflicts since both set and increase independently .

Copy link
Contributor

Choose a reason for hiding this comment

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

A common counter will handle that

@@ -308,6 +317,9 @@ func main() {
return
}
result := rh.Get(id)
if r.URL.Query().Get("error") == "true" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should work transparently

@SuperQ
Copy link
Member

SuperQ commented Oct 30, 2019

It would be nice to get this done, being able to log all errors rather than just all results would be pretty useful.

@brian-brazil
Copy link
Contributor

Obsoleted by #517

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.

4 participants