search

SAP / cf-java-logging-support

The Java Logging Support for Cloud Foundry supports the creation of structured log messages and the collection of request metrics
Apache License 2.0
77 stars 48 forks source link

TenantId not taken from Request #52

Open WeberAndrea opened 5 years ago

WeberAndrea commented 5 years ago

we want to use the field tenant_id in the MDC properties to add our tenant id to the logger. So far we set the tenant id to the MDC properties by ourselves in a own servlet filter. Would it be possible to add the tenant id automatically to the MDC properties from the Request, like it is already done for the correlation id? This would be very helpful for our applications

KarstenSchnitter commented 5 years ago

This is implemented in the RequestLoggingFilter, which calls the RequestRecordFactory. Since "tenant_id" is a propagated HttpHeader, the RequestLoggingFilter will extract the value of the HTTP header tenantid and put the value in the MDC. All you have to do is to instrument your Servlet with the RequestLoggingFilter. Please also have a look at https://github.com/SAP/cf-java-logging-support/issues/47, which introduced this feature.

HrFlorianHoffmann commented 5 years ago

Taking it from the request header tenant_id is nice, but gets us nowhere. We're using SAP's default XSUAA that encodes the tenant ID as part of the JWT received in a different header field. Is there no support for reading the tenant ID from elsewhere, esp. SecurityContext etc.?

KarstenSchnitter commented 5 years ago

Currently the library offers no special integration with Spring. Therefore, it is unaware of the SecurityContext. Supporting JWT would require access to the secret for decoding and knowledge about the field containing the tenant_id. We need to investigate, if this can be done by a generic implementation fitting this library. For now, you need to explicitly set the tenant_id either directly in the MDC or using our LogContext like this:

LogContext.add(Fields.TENANT_ID, "your-tenant-id");

I am looking forward to more discussion on this topic.

KarstenSchnitter commented 4 years ago

Since there are no further comments, I close this issue.

MahatmaFatalError commented 4 years ago

Are there any plans to integrate the tenant logging from the JWT token in the future?

KarstenSchnitter commented 4 years ago

@MahatmaFatalError have a look at #69 and #82. you can easily implement your own extension to the DynamicLogLevel Processor, that does this kind of things with that change.

prateekprshr-nith commented 3 years ago

Hi, In my opinion, most SAP applications running on cloud foundry utilize SAP's XSUAA itself. Doesn't it make sense to implement this feature (automatically taking tenant_id from JWT token) for such a large chunk of applications? (I am pretty sure that majority of applications making use of cf-java-logging-support, running on cloud foundry, utilize XSUAA)

KarstenSchnitter commented 3 years ago

@prateekprshr-nith, I agree, that in that context this would be most helpful. However, it would introduce a lot of additional dependencies to this library. I wonder, if there is not some higher level framework for your use-case, e.g. a buildpack, that combines this library with the uaa access library? This would be a perfect place to include the integration you are looking for.

prateekprshr-nith commented 3 years ago

Unfortunately, we use sap_java_buildpack (again vast majority of java applications running on cloud foundry use this) and it doesn't provide such capabilities. Are there any issues in introducing a lot of additional dependencies (not sure if lot of libraries will be required 😀) to this library, for a feature that will be consumed by majority consumers of this library? I am sure that additional dependencies will be from SAP's XSUAA itself and they can be trusted.

KarstenSchnitter commented 3 years ago

I have looked into supporting this in the past. Back then, I would need to access a Spring Security context. Which would bring Spring as an additional dependency. This is rather a lot. It would basically require its own Maven module. This seems to be rather a lot for a very small interceptor class you need to write in your application.

MahatmaFatalError commented 3 years ago

How about introducing a new spring-boot-starter-cf-java-logging lib that comprises cf-java-logging-support and does provide the out-of-the-box functionality for the vast majority of consumers?

prateekprshr-nith commented 3 years ago

I have looked into supporting this in the past. Back then, I would need to access a Spring Security context. Which would bring Spring as an additional dependency. This is rather a lot. It would basically require its own Maven module. This seems to be rather a lot for a very small interceptor class you need to write in your application.

I don't see any harm in adding Spring as an additional dependency too, in a library which is consumed by all SAP java applications running on cloud foundry. On the contrary, requiring all those applications / microservices (easily in hundreds or in thousands) to implement same repetitive thing, however small, is just a duplicated effort for all of the teams running and maintaining those applications.

rahuldeepattri commented 3 years ago

+1

I don't see any harm in adding Spring as an additional dependency too, in a library which is consumed by all SAP java applications running on cloud foundry. On the contrary, requiring all those applications / microservices (easily in hundreds or in thousands) to implement same repetitive thing, however small, is just a duplicated effort for all of the teams running and maintaining those applications.

calle2010 commented 3 years ago

Not sure what you mean by SAP Java applications. If it means Java applications written by SAP and using CAP you may be right. But there are also customers and partners, and not everybody may be happy about such a choice. Even if my own application uses Spring I don't want to be forced into specific versions by a small dependency like this, so this is adding more complexity.

Therefore -1 for a Spring dependency.

prateekprshr-nith commented 3 years ago

I think trying to avoid adding a dependency and not implementing a feature because of this reason, is quite dogmatic. I don't think it will force anyone to use specific versions of Spring. Applications have 10s and 100s of dependencies and they will be fine with any major stable version of spring and so will be this library. Consuming specific version of a library in an application doesn't add more complexity, it's how this mechanism of dependency management works and it's part and parcel of development.

However the point of customers and partners not being happy is also not convincing. It can be easily resolved by keeping both approaches of getting tenant_id - getting from http header or getting from jwt token. Getting tenant_id from http header can be the primary strategy and getting tenant_id from jwt token can be a fallback. I don't think this will make anyone unhappy. But it surely will make an additional bunch of developers happy.

KarstenSchnitter commented 3 years ago

Hi everybody,

thanks for the lively discussion so far. I understand, that there is a need for tighter integration with Spring. Especially with regards to the tenantId. However, the point raised by @calle2010 is a very valid one:

Even if my own application uses Spring I don't want to be forced into specific versions by a small dependency like this, so this is adding more complexity.

It also shows, that there is a problem for maintaining this library: Choosing the right version to support and keeping up with newer versions. Hopefully the library code will be compatible with many Spring versions, but that is not guaranteed.

The way towards a common solution is outlined by @MahatmaFatalError in my opinion:

How about introducing a new spring-boot-starter-cf-java-logging lib that comprises cf-java-logging-support and does provide the out-of-the-box functionality for the vast majority of consumers?

This would mean to add a new Maven submodule to the library, e.g. cf-java-logging-support-spring, that can optionally be used. It would contain Spring integration and the (runtime) dependencies. Besides the tenantId, there are more integrations to Spring, that could ease the use of cf-java-logging-support. Support for tenantId would be a good starting point for that.

To get to this place, we need your support. Sharing code examples and detailed explanations of what you want to see, would be very much appreciated. Please reach out directly to me, if you do not want to do this over public Github. I can start a branch with a Maven skeleton, if you want to contribute PRs as well. Please let me know, what you need and how you think we go about this.

Best Regards, Karsten