spring-cloud / spring-cloud-stream

Framework for building Event-Driven Microservices
http://cloud.spring.io/spring-cloud-stream
Apache License 2.0
1k stars 610 forks source link

Add Spring Cloud Stream version as part of message headers produced #2814

Open sobychacko opened 1 year ago

sobychacko commented 1 year ago

Adding the Spring Cloud Stream version as part of the message header is convenient for easier debugging of issues.

omercelikceng commented 1 year ago

@sobychacko If the issue is available, I would like to try to solve it. Let me give you brief information on how to do it.

Using the spring boot maven plugin, I will create the build-info.properties file under the META-INF directory at build time. Then I will inject this resource as a bean. And before sending the data, I will add it to the header. If you don't like the solution, I can think of another solution.

sobychacko commented 1 year ago

@omercelikceng You can certainly work on the issue if you have the bandwidth. I am not sure if we want to use the boot maven plugin that way at build time in the framework. Usually, we don't include the boot maven plugin as part of the framework build, but is used by the applications to create a boot app. We don't want to accidentally create uber-jars from the framework. We probably can get this information differently and attach that to the jar manifest.

omercelikceng commented 1 year ago

@sobychacko OK, I'll think of another way. But I didn't understand what you mean in the following sentence.(Word I don't understand: bandwidth) Did you mean to ask if I've worked on this topic before?

You can certainly work on the issue if you have the bandwidth.

sobychacko commented 1 year ago

You can certainly work on the issue if you have the bandwidth.

I meant, if you have the time, go ahead and work on the issue:-)

omercelikceng commented 1 year ago

Bant genişliğiniz varsa kesinlikle sorun üzerinde çalışabilirsiniz.

Demek istediğim, vaktiniz varsa devam edin ve konu üzerinde çalışın :-)

Ok thank you very much. Sorry for my bad English knowledge. :) I still need to learn some words. Please excuse me. I will work on this issue and write you my opinion.

sobychacko commented 1 year ago

No worries. Please let us know if you have any questions on this issue.

omercelikceng commented 1 year ago

@sobychacko Hello. I have completed my development and I would like to ask a few questions.

While the project is being built, I save the relevant Spring Cloud Stream version in the Manifest.mf file. And I get the Spring Cloud Stream version from this file. Since I found a solution using the manifest file, I tried to try all the build methods. (fat jar, shaded jar, war vb.)

My code works successfully in the following cases. 1- I create a fat jar with spring-boot-maven-plugin 2- I create a war with spring-boot-maven-plugin.

But it is necessary to consider the following situations in detail. 1- If I create a shaded jar with the shaded plugin, the MANIFEST file is created only for the main project. And since there is no MANIFEST file specific to the spring cloud stream jar, I cannot get the correct version. I saw that in a shaded jar, instead of a MANIFEST file, a pom.properties file specific to the spring cloud stream jar is created. So I can use the pom.properties file. However, when I create a shaded jar, a MANIFEST file is not created for the spring cloud stream because we are in a third party position.

However, special situations may occur here. This pom.properties file may not be created depending on how the developer builds. So, from what I have observed, it seems that it is not right to trust MANIFEST.MF or pom.properties files. What path should I follow? I saw that there are plugins that generate a class that contains a version. Maybe if I use these it would be a better solution. Can you help me?

Or should I give the developer a message like the one below and ignore the situations I explained? "No manifest file or pom.properties file found in the jar. Please review the build method."

sobychacko commented 1 year ago

@omercelikceng Sorry for the delay in replying to you. I see that this is not a trivial problem to solve. I wonder if there is a more straightforward way to address this rather than going with the manifest route. We will explore if there is a solution and get back to you. In the meantime, please let us know if you can think of a better solution.

omercelikceng commented 1 year ago

@sobychacko.. No problem. I researched and found another solution. I can generate a class containing the version using the maven-antrun-plugin at compile time. What do you think about this solution? However, in this solution, the compile time of spring-cloud-stream will increase slightly.

sobychacko commented 1 year ago

@omercelikceng That might not be a bad idea. How intrusive is the code for doing that? I want to keep the changes for this very minimal. @artembilan and I have briefly discussed this issue today, and he was also thinking about something along these lines, i.e., coming up with a static properties file with the version.

olegz commented 1 year ago

We can get this info by parsing the JAR file name. We control it and we know that version is always going to be part of it.

URL url = AbstractBinder.class.getProtectionDomain().getCodeSource().getLocation();
// parse the url
artembilan commented 1 year ago

See also org.springframework.core.SpringVersion impl.

It is probably may fail in case of shading, however in this case we may just fall back and don't provide such a null value for SCST version header. Just because it is not a standard SCST artifact, you are on your own to supply that value yourself.

omercelikceng commented 1 year ago

We can get this info by parsing the JAR file name. We control it and we know that version is always going to be part of it.

URL url = AbstractBinder.class.getProtectionDomain().getCodeSource().getLocation();
// parse the url

Hello @olegz. Thank you very much for your interest. Actually, I developed my code as you said. But when I do it this way, the problems I mentioned above occur when it is a shaded jar.

Problems:

2814 (comment)

omercelikceng commented 1 year ago

See also org.springframework.core.SpringVersion impl.

It is probably may fail in case of shading, however in this case we may just fall back and don't provide such a null value for SCST version header. Just because it is not a standard SCST artifact, you are on your own to supply that value yourself.

Hello @artembilan. Thank you very much for your interest. I couldn't fully understand some of what you wrote. Please excuse me if I have rewritten what you already wanted to say. Just wanted to check.

I can do it this way. But when I generate shaded jar, the version is not null, we get the version of the main project. So I can't get the version of spring cloud stream. In this case, I need to check to understand that the value I got is the wrong version(main project version). I can do this like this(implementationTitle = "spring-cloud-stream"). But I'm not sure if this is a reliable way.

But I can do whatever you say. Could you please check me? I will get the version as in the org.springframework.core.SpringVersion method. If I cannot get the version(shaded-jar), I will not publish the version information in the header. Should I implement the implementation this way?

We gave up on implementing it this way, right?(https://github.com/spring-cloud/spring-cloud-stream/issues/2814#issuecomment-1768998447)

artembilan commented 1 year ago

I see that SpringBootVersion has it explicitly set. The source code for that is like this: https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot/src/main/javaTemplates/org/springframework/boot/SpringBootVersion.java.

They have a Gradle task for that template: https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot/build.gradle#L153-L159

So, you might be not wrong going with something what could be done from Maven to modify such a template on project build. I doubt that you do shading against source code, so this one should be reliable way to go.

However the final solution is up to @olegz as he is a lead of the project.

omercelikceng commented 1 year ago

@artembilan You gave this example (org.springframework.core.SpringVersion). https://github.com/spring-cloud/spring-cloud-stream/issues/2814#issuecomment-1772886764 Now you give this example. (org.springframework.boot.SpringBootVersion). https://github.com/spring-cloud/spring-cloud-stream/issues/2814#issuecomment-1773169127 These are completely different implementations. Which one do you mean?

artembilan commented 1 year ago

We are still gathering ideas how to implement this feature. I'm fine with both variants. And I'd prefer the the SpringVersion since I don't believe that shading is a good practice. Especially for this framework which is just not a simple library to be used a part of other framework you may shade for: we build or single uber jar based on Spring Boot, or native image. Why would one go the route for shading of this stuff?..

omercelikceng commented 1 year ago

Developers may not be able to use uber jar due to problems with the third party libraries they use. I currently have an example like this in my project.(If you want, I can explain the example in my project.) I'm trying to have a code that works in every case. But the final decision is yours, of course.

olegz commented 1 year ago

This works in every case as shown earlier

URL url = AbstractBinder.class.getProtectionDomain().getCodeSource().getLocation();
omercelikceng commented 1 year ago

Hello @olegz. I tried again. But what you said doesn't work in shaded jar. Because in shaded jar, all jars are extracted. And it is not available as spring cloud stream jar. If you want, I can share a poc showing that this doesn't work in shaded jar.

Or I can ignore this situation and develop it as you say.

olegz commented 1 year ago

You can also try to explore reading it from the MANIFEST

Manifest-Version: 1.0
Created-By: Maven JAR Plugin 3.2.2
Build-Jdk-Spec: 17
Implementation-Title: spring-cloud-stream
Implementation-Version: 4.1.0-SNAPSHOT
Implementation-Vendor: Pivotal Software, Inc.

However not sure what manifest you'll see once the whole thing is shaded, but give it a shot and see what's in the manifest. We have plenty of samples how to read one from a zipped file

Other then that, let me think a bit about it

omercelikceng commented 1 year ago

Ok. I wrote about the problems that occur when shaded jar occurs here(https://github.com/spring-cloud/spring-cloud-stream/issues/2814#issuecomment-1741747667). As you said, the problems that occur when I try to read from the manifest file are listed here.

That's why I offered to generate classes. (https://github.com/spring-cloud/spring-cloud-stream/issues/2814#issuecomment-1768998447)

But @artembilan also thinks like this. (https://github.com/spring-cloud/spring-cloud-stream/issues/2814#issuecomment-1773181360)

I couldn't decide what I should do last. I need your guidance.

olegz commented 1 year ago

Well SpringVersion would suffer the same manifest issue as any other during shading, right? And even if there is some boot magic it would still only give you Spring framework version (not stream or function). SpringVersion also talks about it not being 100% reliable due to class loaders not exposing package metadata. So the only guaranteed way is to read it from the JAR file name which we control. But as you said it will present issue with shading.

So we can either state that we would not support this feature if shaded or we can consider some type of Maven plugin/extension that would generate a class on evert build that we would reference to get the version. I'd be interested to see what it would look like. In other words we are back to your original proposal, so give it a shot

sobychacko commented 1 year ago

I agree with what @olegz said, we only need to support this feature when using the boot plugin and won't support shade or other plugins.

omercelikceng commented 11 months ago

Hello, I haven't had time for the project for a long time. We are in a busy period at my company. I won't be able to spare time for this issue for a while.(maybe a month) Would there be any problems for you?

sobychacko commented 11 months ago

@omercelikceng No worries. Let us know when you are able to pick this up again. Otherwise, we can take a look from our side.

omercelikceng commented 3 months ago

Hello, I am sorry. I've been working really hard for a while now. That's why I couldn't spare much time. But I have completed my development. However, we need to resolve this issue(https://github.com/spring-cloud/spring-cloud-stream/pull/2983) before I can create the pull request because the changes I will make will also affect this part. Therefore, I cannot create the pull request. However, I wanted to let you know that I have completed the development.

olegz commented 3 weeks ago

@omercelikceng given that #2983 is resolved would you like to finish this?

omercelikceng commented 3 weeks ago

Hello @olegz. I completed my development three months ago, but I have been very busy for a while now. Additionally, I have discovered some issues in the project that are similar to the one in https://github.com/spring-cloud/spring-cloud-stream/pull/2983#issuecomment-2275384664.

I believe it would be more appropriate to tackle these header-related problems first. If you permit, I would like to resolve the other issues related to the headers before proceeding. After that, I would like to submit a pull request for this development.

I also noticed and resolved the issue mentioned in https://github.com/spring-cloud/spring-cloud-stream/issues/2869.

However, I would like to request 1-2 months of time to complete everything and submit a comprehensive pull request. If you want me to solve the problems faster, please tell me. I will do it.