Update OtelLogStorageFactory to use last workflow-api changes
#1176
Conversation
| <jenkins.baseline>2.504</jenkins.baseline> | ||
| <jenkins.version>${jenkins.baseline}.1</jenkins.version> |
There was a problem hiding this comment.
This bump is required due to the workflow-api bump.
We can file this bump as a separate PR if requested.
| <exclusion> | ||
| <!-- provided by org.jenkins-ci.plugins:jackson2-api --> | ||
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-core</artifactId> | ||
| </exclusion> | ||
| <exclusion> | ||
| <!-- provided by org.jenkins-ci.plugins:jackson2-api --> | ||
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-databind</artifactId> | ||
| </exclusion> |
There was a problem hiding this comment.
Fix some "require upper bounds" maven issues.
pom.xml
Outdated
| <exclusion> | ||
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-compress</artifactId> | ||
| </exclusion> |
There was a problem hiding this comment.
Need to remove this exclusion due to https://issues.jenkins.io/browse/JENKINS-73355.
This dependency is mandatory for testcontainers.
This library has a test scope and is not bundled in the final HPI.
| * <p> | ||
| * See <a href="https://github.com/jenkinsci/pipeline-cloudwatch-logs-plugin/blob/pipeline-cloudwatch-logs-0.2/src/main/java/io/jenkins/plugins/pipeline_cloudwatch_logs/PipelineBridge.java">Pipeline Cloudwatch Logs - PipelineBridge</a> | ||
| */ | ||
| @Extension |
There was a problem hiding this comment.
With the Describable/Descriptor pattern, the @Extension is not supported anymore on the factory.
| System.setProperty("org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep.USE_WATCHING", "true"); | ||
| } | ||
|
|
||
| @Inject |
There was a problem hiding this comment.
We can't inject anymore with the @Extension removal.
There was a problem hiding this comment.
We use @Inject ~30 times in this plugin, is there a problem now with @Inject?
There was a problem hiding this comment.
It was only a problem here because LogStorageFactory is no longer an ExtensionPoint after the upstream changes and so the implementations are no longer singleton @Extensions and so dependency injection doesn't work. In other contexts, @Inject is fine.
| @PostConstruct | ||
| public void postConstruct() { | ||
| this.tracer = jenkinsControllerOpenTelemetry.getDefaultTracer(); |
There was a problem hiding this comment.
Useless with the @Extension removal.
| } | ||
|
|
||
| @Extension() | ||
| @Symbol("openTelemetry") |
There was a problem hiding this comment.
Describable/Descriptor pattern, the symbol "openTelemetry" is used for CasC configuration.
| @NonNull | ||
| @Override | ||
| public String getDisplayName() { | ||
| return "OpenTelemetry"; |
There was a problem hiding this comment.
Displayed in the new "Pipeline logging" global UI configuration, see jenkinsci/workflow-api-plugin#417
| static OtelLogStorageFactory get() { | ||
| return ExtensionList.lookupSingleton(OtelLogStorageFactory.class); | ||
| } |
| System.setProperty("org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep.USE_WATCHING", "true"); | ||
| } | ||
|
|
||
| @Inject |
There was a problem hiding this comment.
We use @Inject ~30 times in this plugin, is there a problem now with @Inject?
|
@kuisathaverat Cyrille took a look at this and seems to be ok with it, but what do you think? Are you ok with the general idea of the API changes? Are you interested in having us help clean up all of the |
With great pleasure, thanks for the offer. |
|
@cyrille-leclerc Ok, does that mean you're planning on merging and releasing this PR? We'll need a coordinated release of this PR soon after jenkinsci/workflow-api-plugin#417 is released to avoid widespread breakage. We can mention the incompatibility in release notes and tell people not to update, but we'd want the release of this PR to go out roughly the same day as the upstream PR, or at least the next day. Let us know how you'd prefer to coordinate that. |
kuisathaverat
left a comment
kuisathaverat
left a comment
There was a problem hiding this comment.
this is a breaking change in the log API, I want to test it manually to verify any configuration is affected.
|
Hello @kuisathaverat , did you have the chance to check the changes? 🙏🏼 |
4 participants
jenkinsci/workflow-api-plugin#417 changes the
LogStorageFactoryAPI in an incompatible way, see that PR for details and justification.This PR adapts to the upstream changes, and will need to be released roughly at the same time as that PR.
We are opening this draft PR to make sure the maintainers here are ok with the general idea and to help coordinate the release.
This PR applies the following changes:
OtelLogStorageFactoryto use theDescribable/Descriptorpattern, which requires some minor changes due toOtelLogStorageFactoryno longer being a singleton@Extensionpom.xmlas required by theworkflow-apiupdate, which requires some changes to fix dependency issues and upper bounds conflicts. We can file these updates as a standalone PR if desired.As the upstream changes allow users to configure functionality equivalent to
otel.logs.mirror_to_disk=truemechanism in a generic way (other than the caveat of no longer being able to print a link to the observability platform into the logs dynamically), the current plugin won't need theTee*classes anymore and the logging code can be simplified if desired. We can handle this simplification in a followup PR if you are interested.Testing done
Submitter checklist