Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow configurability of the middleware used by the HttpEventCollectorSender #260

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

simonhege
Copy link
Contributor

A user application can utilize HttpEventCollectorMiddleware to customize the behavior of sending events to Splunk.
This can be configured by using the "middleware" configuration key

Thread.currentThread().getContextClassLoader().loadClass(config.middleware.get())
.asSubclass(HttpSenderMiddleware.class)
.getDeclaredConstructor()
.newInstance());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am bit sceptical this is gonna work with native compilation : The new instance of the middleware is done by reflection which is not supported by native image without the proper metadata: native image need to known all classes used at runtime otherwise it tries to remove every class definition not used int the final executable.

So we need to instrument the extension to retain the class definition, for that we need to produce a ReflectiveClassBuildItem from the LoggingSplunkProcessor: this might require a build time config which makes sense because the middleware is known at build time.
I would also provide a test in the integration-test package: we are performing native compilation there to test the extension.

@simonhege
Copy link
Contributor Author

I was more thinking that the class to be used has to be annotated with @RegisterForReflection. Especially on our side we plan to override this middleware config part of our deployment. Yes, possible classes are known at build time, but not the exact value of the middleware config parameter.

Do you suggest to use something like combinedIndex.getIndex().getAllKnownSubclasses(HttpSenderMiddleware.class) so that user does not care about using @RegisterForReflection?

@vietk
Copy link
Contributor

vietk commented Jul 2, 2024

I was more thinking that the class to be used has to be annotated with @RegisterForReflection. Especially on our side we plan to override this middleware config part of our deployment.

Yes it's a solution.

Yes, possible classes are known at build time, but not the exact value of the middleware config parameter.

I had the feeling that even the name of the middleware is known at build time, can you explain why it would be a runtime configuration?

Do you suggest to use something like combinedIndex.getIndex().getAllKnownSubclasses(HttpSenderMiddleware.class) so that user does not care about using @RegisterForReflection?

It could be a solution if we agree on point 2

@simonhege
Copy link
Contributor Author

Yes, possible classes are known at build time, but not the exact value of the middleware config parameter.

I had the feeling that even the name of the middleware is known at build time, can you explain why it would be a runtime configuration?

In our CI/CD we aim at deploying same built version in every environment. Then step by step activate a change in one environment, then the other up to production.
But we can accommodate this and handle this environment-specific activation inside the middleware we plan to use.

@vietk
Copy link
Contributor

vietk commented Jul 2, 2024

But we can accommodate this and handle this environment-specific activation inside the middleware we plan to use.

Fair enough, so let's try the solution proposed above

@vietk
Copy link
Contributor

vietk commented Jul 3, 2024

Fine for me.

@vietk vietk merged commit 04cdda3 into quarkiverse:main Jul 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants