Punycode pipeline names #33
Conversation
collector/collector.go
Outdated
| @@ -196,7 +197,7 @@ func (c *Collector) addBuildAndJobMetrics(r *Result) error { | |||
| continue | |||
| } | |||
|
|
|||
| pipeline := *build.Pipeline.Name | |||
| pipeline, _ := idna.ToASCII(*build.Pipeline.Name) | |||
There was a problem hiding this comment.
Probably a good idea to check this error and return it if it occurs! Who knows what dragons live here :)
|
This looks great! Thank you so much! |
collector/collector.go
Outdated
| @@ -196,7 +197,7 @@ func (c *Collector) addBuildAndJobMetrics(r *Result) error { | |||
| continue | |||
| } | |||
|
|
|||
| pipeline := *build.Pipeline.Name | |||
| pipeline, _ := idna.ToASCII(*build.Pipeline.Name) | |||
There was a problem hiding this comment.
I'm not sure if we should be more worried about errors here. The only error case I can think of is malformed UTF-8, which seems like a minimal risk given that we've already read the string in to Go.
There was a problem hiding this comment.
If we do get an error it will result in a blank string for pipeline though, which will be a whole lot harder to troubleshoot than an explicit error.
| Pipeline: &bk.Pipeline{Name: bk.String("vicuñas")}, | ||
| State: bk.String("scheduled"), | ||
| Jobs: []*bk.Job{ | ||
| {State: bk.String("scheduled"), AgentQueryRules: []string{"queue=default"}}, |
There was a problem hiding this comment.
I added this to the default queue because I couldn't find a way to add another; this means I had to change a bunch of other, unrelated numbers below. Is there a better way to do this?
There was a problem hiding this comment.
That test needs some cleaning up to split apart those concerns, but I'm fine with it for now.
rbvigilante
force-pushed
the
punycode-pipeline-names
branch
from
25fb4a5 to
12d0b5b
Compare
collector/collector.go
Outdated
| pipeline, ucError := idna.ToASCII(*build.Pipeline.Name) | ||
|
|
||
| if ucError != nil { | ||
| return false |
There was a problem hiding this comment.
Again, not entirely sure what the idiomatic thing to do here is; I'm guessing false is a failure?
There was a problem hiding this comment.
That gives me
collector/collector.go:203:5: cannot use ucError (type error) as type bool in return argument
There was a problem hiding this comment.
Apologies, code reading comprehension fail! I'd go for:
if usErr != nil {
log.Printf("Error converting pipeline to ascii: %v", usErr)
continue
}There was a problem hiding this comment.
FWIW err is usually idiomatic in golang.
There was a problem hiding this comment.
I was just reading the code for better comprehension myself. I guess the other option would be to make the function passed into Pages return a bool, error instead of just a bool and handle it there. Then we can get what I think is the desired effect of a crash-out rather than just losing some metrics?
There was a problem hiding this comment.
It absolutely could, but it probably should continue to process subsequent pipelines, which is why errors are handled elsewhere with continue.
There was a problem hiding this comment.
Ah, so we'd prefer to just lose some metrics (which I guess is what will happen if we just log and continue? Forgive my ignorance if continue does something special to do with error handling in Go). Will amend now!
There was a problem hiding this comment.
Yup, that was generally my thinking. continue is just the usual for loop construct that will continue to the next loop iteration!
This prevents CloudWatch from complaining about pipelines with non-ASCII characters in their names. We thought it made more sense to do it on collection rather than in the backend to maintain consistency of names across backends
rbvigilante
force-pushed
the
punycode-pipeline-names
branch
from
12d0b5b to
106a76d
Compare
|
Looks great! Thanks so much! I will get this into a new release of the elastic stack as quick as I can! |
We aren't necessarily amazing at Go, so apologies if we've done something anti-idiomatic.
This runs punycode over all pipeline names as metrics are collected, which should solve #32