spring-cloud / spring-cloud-commons

Common classes used in different Spring Cloud implementations
Apache License 2.0
707 stars 704 forks source link

Log output during /actuator/refresh call #1311

Closed litepl closed 4 months ago

litepl commented 11 months ago

Is your feature request related to a problem? Please describe. When an "old-style" reload finds some changes in configmap o secret it puts some info into the log like:

2023-11-16 13:05:26,718 - xxx-cc77b79d5-dfr4l - INFO  [pool-16-thread-1 - trace=N/A, span=N/A] o.s.c.l.LogAccessor: Detected change in config maps

calling /actuator/refresh says nothing in the log

Describe the solution you'd like It would be nice to have such a log when calling /actuator/refresh endpoint

Describe alternatives you've considered None

Additional context None

wind57 commented 11 months ago

I don't know about this... you can write your own listener and catch RefreshScopeRefreshedEvent, right?

wind57 commented 10 months ago

I was not too verbose on the above comment, but IMO, I don't understand what log statement you would want.

When we log: "Detected change in config maps" for example, we know that a change has happened, as such we issue that log statement. But in case of calling /actuator/refresh - what would you expect to happen? Something along the lines of :

refresh endpoint called, reloading configmap/secret based property sources.

If so, then why can't you do it in your app? You will need something along the lines of:

class MyListener implements ApplicationListener<RefreshScopeRefreshedEvent> { 
     @Override
    public void onApplicationEvent(RefreshScopeRefreshedEvent event) {
        // log whatever you want
    }
}

register this one as a @Bean/@Component/etc and you are good. We could probably add it too, but that might be, or not be a good option.

@ryanjbaxter thoughts?

ryanjbaxter commented 10 months ago

Wouldn't this log statement do the same thing? https://github.com/spring-cloud/spring-cloud-kubernetes/blob/main/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/config/reload/ConfigurationChangeDetector.java#L50

wind57 commented 10 months ago

I don't think that this is what OP is asking for. That log statement still comes when we receive an event from kubernetes about the state of a certain configmap.

What they want is a log statement when /actuator/refresh is called directly, like with curl for example. This is how I read this, at least.

litepl commented 10 months ago

Exactly. That's because of migration to Spring Cloud Configuration Watcher which calls /actuator/refresh.

wind57 commented 10 months ago

@litepl I think you should have mentioned that in the initial description. We have a bunch of logs from configuration watcher side for this. At the moment they are on debug mode though. For example, this this log statement.

But again, that is in the configuration watcher, not your application. Will that suite your needs?

litepl commented 10 months ago

Not exactly. To me, it'd be better to have that information in the app's log, not in sckcw ones. It's easier to create alerts for that as an example.

ryanjbaxter commented 10 months ago

So you want a log statement here? https://github.com/spring-cloud/spring-cloud-commons/blob/main/spring-cloud-context/src/main/java/org/springframework/cloud/endpoint/RefreshEndpoint.java#L41

litepl commented 10 months ago

Yes. Not I want to, I would like to :D It's a kind request

wind57 commented 10 months ago

I am still inclined not to do this. A log statement there does nothing useful, imho; but if someone really needs it, its really easy to write your own Listener as outlined above.

Especially since a call to /refresh might come from some other caller, not only the configuration watcher. We can't even distinguish where the call comes from, as such the log statement can not even contain "configmaps" or "secrets" words.

We do have proper logs that would make far more sense and they come from the configuration watcher, from the source. If alerts can be created on the app, then Im sure alerts can be created on the watcher also.

This is my view on this issue, at least at the moment :)

ryanjbaxter commented 10 months ago

Well this is not the repo for the request as the code that handles the /refresh endpoint lives in Spring Cloud Commons. It is not specific to Kubernetes its any Spring Cloud application. If you would like, you can submit a PR to add the log statement and we can see what the rest of the team thinks.