Use recommended labels #172
Conversation
|
This change is |
|
This is the first draft that would fix #170, intended to get a consensus on the names/values for the labels. @harshal-shah, do you think something like this would be suitable for your use-case? |
Codecov Report
@@ Coverage Diff @@
## master #172 +/- ##
==========================================
+ Coverage 96.38% 96.46% +0.07%
==========================================
Files 30 30
Lines 1438 1469 +31
==========================================
+ Hits 1386 1417 +31
Misses 40 40
Partials 12 12
Continue to review full report at Codecov.
|
| func (c *Collector) labels() map[string]string { | ||
| return map[string]string{ | ||
| "app.kubernetes.io/name": c.name(), | ||
| "app.kubernetes.io/instance": c.jaeger.Name, |
There was a problem hiding this comment.
Looks reasonable - only one I wasn't sure about was the instance, where the docs describe it as "A unique name identifying the instance of an application" with an example of wordpress-abcxzy. But for our purpose it enables objects to be selected by various criteria.
There was a problem hiding this comment.
I'll apply the same change to the other objects then. Thanks!
There was a problem hiding this comment.
Don't forget we will also need to support the app label for a while - was thinking that should be same as app.kubernetes.io/name value?
There was a problem hiding this comment.
Do you have existing applications in mind, or is there a case of an application that still reads the app instead of app.kubernetes.io? If it's about maintaining backwards compatibility, we need to keep app: jaeger.
There was a problem hiding this comment.
I guess the two areas to consider is:
- Istio - which needs the
applabel andapp:jaegershould be fine - Kiali - raised by Jay? where unique
applabel seemed to be a problem - but possibly this is not a blocker
There was a problem hiding this comment.
If they are also moving to use app.kubernetes.io instead of app, then I'd keep app: jaeger for backwards compatibility.
There was a problem hiding this comment.
Unclear what the timeframe is for them changing, but yes probably the best approach.
jpkrohling
force-pushed
the
170-Use-Recommended-Labels
branch
from
2840a24 to
b3e69ef
Compare
|
This is now ready to be reviewed. |
|
Tests added |
|
Thanks 👍 |
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling
force-pushed
the
170-Use-Recommended-Labels
branch
from
6f6aa68 to
79acff3
Compare
This PR replaces the
selectorwith a broaderlabelsmap, following the Kubernetes' recommended labelsSigned-off-by: Juraci Paixão Kröhling [email protected]