OSFUSE-391: f-m-p annotates services with prometheus.io annotations by default #623
Conversation
There was a problem hiding this comment.
Looks good to me. I wonder whether we shouldn't put the service port here as well ? Then we would everything Prometheus related in a single place which can easily be switched on and off.
Don't know though how easy this is because one would need to examing an exsiting service (it safe to assume that the prometheus enricher comes after after e.g. a default service creation).
Also I'm it might make sense to introduce two phases for enrichers: One 'create' phase where missing objects are created and an 'adapt' phase which only tune up existing objects. wdyt ?
Also, some unit test (I know we dont have that much yet, but enricher should be easy to unit test) would be super awesome as well a some docs (small paragraph with the possible config options).
| } | ||
|
|
||
| private List<ImageConfiguration> getImageConfigurations() { | ||
| List<ImageConfiguration> images = getContext().getImages(); |
There was a problem hiding this comment.
images should be always non-null (i.e. an empty list if nothing is configured)
lburgazzoli
force-pushed
the
OSFUSE-391
branch
from
f3a2233
to
48031be
Compare
|
Yes, I think it would be nice to have Prometheus related stuff in a single place but this could be generic enough so that if we add new stuffs, they can be handled the same way. I'm wondering if it could make sense to have a programmatic way to hook enrichers to a phase/event/object so that an enricher is triggered just after a service has been created. |
lburgazzoli
force-pushed
the
OSFUSE-391
branch
from
48031be
to
f3def0f
Compare
|
Added some small unit tests |
lburgazzoli
force-pushed
the
OSFUSE-391
branch
from
f3def0f
to
c694974
Compare
|
Added minimal dock |
|
Thanks ! will merge it that evening. w.r.t to enricher hooks: This is definitely something to consider. Up to now a lot of responsibility is put on the enricher to not mess up things. Maybe a bit more controlled flow would help, e.g. by splitting up into to phases as said, and then, when thing get created maybe some hooks. However, when enricher are modifying or 'enriching' existing objects, they possible want to do this on freshly created or already existing/configured objects. |
| if (kind.isService()) { | ||
| String prometheusPort = findPrometheusPort(); | ||
| if (Strings.isNotBlank(prometheusPort)) { | ||
| log.info("Add prometheus.io annotations: %s=%s, %s=%S", |
There was a problem hiding this comment.
Can you switch this to log.verbose() please ? (can be enabled by setting -Dfabric8.verbose or <verbose>true</verbose>. The reason is, that typically all concrete actions (like the commands in a Dockerfile) are logged with this level. We should probably enhance the interface a bit to provide more context information like the service name. but that's another story ;-)
info() should be reserved for important steps like 'Calling enrichers'.
lburgazzoli
force-pushed
the
OSFUSE-391
branch
from
c694974
to
e9bc60a
Compare
|
done |
|
[merge] |
|
thanks ! |
No description provided.