Refactoring UserAgentPlugin#140712
Conversation
…o refactor-UserAgentPlugin
There was a problem hiding this comment.
logstash-bridge changes initial review:
The https://github.com/elastic/logstash-filter-elastic_integration/ plugin will be broken (hits runtime class not found exceptions) by this change because of class relocations such (UserAgentProcessor, UserAgentParser, etc...). The plugin needs to embed the user-agent-api JAR as I have applied here - https://github.com/elastic/logstash-filter-elastic_integration/pull/408/changes#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R376
elastic_integraiton plugin follows the stack rule by keeping the active branches. if you please let us know which versions/branches this change is going to be included, we will also backport accordingly.
About the logstash-bridge changes, I do not have concern and commented my questions/suggestions.
...idge/src/main/java/org/elasticsearch/logstashbridge/plugins/IngestUserAgentPluginBridge.java
Outdated
Show resolved
Hide resolved
...-bridge/src/main/java/org/elasticsearch/logstashbridge/plugins/IngestCommonPluginBridge.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Refactors the former ingest-user-agent module into a user-agent module and exposes user-agent parsing as a node-wide service via plugin DI, while keeping the ingest processor available through ingest-common (and updating downstream wiring like logstash-bridge).
Changes:
- Renames/re-homes user-agent parsing code into
modules/user-agentand introduceslibs:user-agent-apias a shared API. - Injects a
UserAgentParserRegistryintoIngestService/Processor.Parameters, wiring it fromNodeConstruction. - Updates tests/builds/bridges to load
user-agentand validate legacy config directory behavior.
Reviewed changes
Copilot reviewed 53 out of 63 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| x-pack/qa/multi-project/core-rest-tests-with-multiple-projects/src/yamlRestTest/java/org/elasticsearch/multiproject/test/CoreWithMultipleProjectsClientYamlTestSuiteIT.java | Loads user-agent module in multi-project REST test clusters. |
| x-pack/qa/multi-project/core-rest-tests-with-multiple-projects/build.gradle | Adds :modules:user-agent to cluster modules for tests. |
| x-pack/plugin/otel-data/src/yamlRestTest/java/org/elasticsearch/xpack/oteldata/OTelYamlTestSuiteIT.java | Switches test cluster module from ingest-user-agent to user-agent. |
| x-pack/plugin/otel-data/build.gradle | Updates module dependency to :modules:user-agent. |
| x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportGetTrainedModelsStatsActionTests.java | Supplies UserAgentParserRegistry.NOOP to updated IngestService ctor in tests. |
| x-pack/plugin/apm-data/src/yamlRestTest/java/org/elasticsearch/xpack/apmdata/APMYamlTestSuiteIT.java | Switches test cluster module from ingest-user-agent to user-agent. |
| x-pack/plugin/apm-data/build.gradle | Updates module dependency to :modules:user-agent. |
| server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTestHelper.java | Supplies UserAgentParserRegistry.NOOP to updated IngestService ctor in tests. |
| server/src/test/java/org/elasticsearch/ingest/SimulateIngestServiceTests.java | Supplies UserAgentParserRegistry.NOOP to updated IngestService ctor in tests. |
| server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java | Supplies UserAgentParserRegistry.NOOP to updated IngestService ctor in tests. |
| server/src/main/java/org/elasticsearch/plugins/UserAgentParserRegistryProvider.java | Introduces SPI for providing a node-wide UserAgentParserRegistry. |
| server/src/main/java/org/elasticsearch/node/NodeConstruction.java | Constructs and injects UserAgentParserRegistry into IngestService. |
| server/src/main/java/org/elasticsearch/ingest/Processor.java | Extends Processor.Parameters with userAgentParserRegistry. |
| server/src/main/java/org/elasticsearch/ingest/IngestService.java | Plumbs userAgentParserRegistry into processor factory parameters. |
| server/src/main/java/org/elasticsearch/common/settings/Setting.java | Adds longSetting(key, fallbackSetting, ...) helper for fallback settings. |
| server/src/main/java/module-info.java | Adds requires org.elasticsearch.useragent.api; to server module. |
| server/build.gradle | Adds API dependency on :libs:user-agent-api. |
| modules/user-agent/src/yamlRestTest/resources/rest-api-spec/test/user-agent/10_basic.yml | Updates module name assertion to user-agent. |
| modules/user-agent/src/yamlRestTest/java/org/elasticsearch/useragent/UserAgentClientYamlTestSuiteIT.java | Renames/package-moves YAML suite IT and simplifies cluster module loading. |
| modules/user-agent/src/test/java/org/elasticsearch/useragent/UserAgentPluginTests.java | Adds unit tests for cache size setting fallback behavior. |
| modules/user-agent/src/test/java/org/elasticsearch/useragent/DeviceTypeParserTests.java | Updates package/resource loading to new module/plugin class. |
| modules/user-agent/src/main/plugin-metadata/entitlement-policy.yaml | Grants read access to both config/user-agent and legacy config/ingest-user-agent. |
| modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentPlugin.java | New module plugin providing a UserAgentParserRegistry service + setting fallback. |
| modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentParserRegistry.java | Implements registry loading built-in + custom regex files from config dirs. |
| modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentParser.java | Implements API parser interface + collector-based parsing path. |
| modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentCache.java | Package move to new module namespace. |
| modules/user-agent/src/main/java/org/elasticsearch/useragent/DeviceTypeParser.java | Package move to new module namespace. |
| modules/user-agent/src/main/java/module-info.java | Declares org.elasticsearch.useragent JPMS module. |
| modules/user-agent/build.gradle | Renames plugin entry point + adds dependency on :libs:user-agent-api. |
| modules/ingest-user-agent/src/main/plugin-metadata/entitlement-policy.yaml | Removes old module entitlement policy (module removed). |
| modules/ingest-user-agent/src/main/java/org/elasticsearch/ingest/useragent/IngestUserAgentPlugin.java | Removes old ingest-user-agent plugin implementation (migrated). |
| modules/ingest-common/src/yamlRestTest/resources/rest-api-spec/test/ingest/420_useragent_custom_regex_legacy.yml | Adds REST test for legacy custom-regex directory support. |
| modules/ingest-common/src/yamlRestTest/java/org/elasticsearch/ingest/common/IngestCommonClientYamlTestSuiteIT.java | Configures cluster with both new + legacy custom-regex config dirs for tests. |
| modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/UserAgentProcessorTests.java | Updates unit tests to obtain parsers via UserAgentPlugin.createRegistry. |
| modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/UserAgentProcessorFactoryTests.java | Updates factory tests to use UserAgentParserRegistry and new config layout. |
| modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/UserAgentProcessor.java | Switches processor to API parser/Details model and registry-based parser selection. |
| modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/IngestCommonPlugin.java | Registers user_agent processor factory using injected registry + metadata upgrader. |
| modules/ingest-common/src/main/java/module-info.java | Adds requires org.elasticsearch.useragent.api;. |
| modules/ingest-common/build.gradle | Adds :libs:user-agent-api and uses :modules:user-agent for tests. |
| libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/VersionedName.java | Introduces API record for name/version pairs. |
| libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/UserAgentParserRegistry.java | Introduces API registry interface (with NOOP). |
| libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/UserAgentParser.java | Introduces API parser interface and default Details-building method. |
| libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/UserAgentParsedInfo.java | Adds ES |
| libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/UserAgentInfoCollector.java | Adds collector interface for allocation-friendly parsing. |
| libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/Details.java | Adds API Details record + collector factory builder. |
| libs/user-agent-api/src/main/java/module-info.java | Renames/exports new org.elasticsearch.useragent.api JPMS module. |
| libs/user-agent-api/build.gradle | Adds publishable user-agent-api library build. |
| libs/logstash-bridge/src/main/java/org/elasticsearch/logstashbridge/plugins/IngestUserAgentPluginBridge.java | Reworks bridge to construct user_agent processor from injected registry (deprecated). |
| libs/logstash-bridge/src/main/java/org/elasticsearch/logstashbridge/plugins/IngestCommonPluginBridge.java | Exposes USER_AGENT_PROCESSOR_TYPE constant for bridge consumers. |
| libs/logstash-bridge/src/main/java/org/elasticsearch/logstashbridge/ingest/ProcessorParametersBridge.java | Creates registry via UserAgentPlugin.createRegistry when building parameters. |
| libs/logstash-bridge/src/main/java/module-info.java | Updates module requirements for new user-agent + API modules. |
| libs/logstash-bridge/build.gradle | Updates compileOnly deps to :modules:user-agent + :libs:user-agent-api. |
| build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/InternalDistributionModuleCheckTaskProvider.java | Adds org.elasticsearch.useragent.api to distribution module checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentPlugin.java
Outdated
Show resolved
Hide resolved
modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentPlugin.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/node/NodeConstruction.java
Outdated
Show resolved
Hide resolved
libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/UserAgentParsedInfo.java
Outdated
Show resolved
Hide resolved
libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/UserAgentParsedInfo.java
Outdated
Show resolved
Hide resolved
modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentPlugin.java
Show resolved
Hide resolved
modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentParserRegistry.java
Outdated
Show resolved
Hide resolved
modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentParserRegistry.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Mashhur <99575341+mashhurs@users.noreply.github.com>
…o refactor-UserAgentPlugin
|
Pinging @elastic/es-distributed (Team:Distributed) |
UesrAgentPluginUesrAgentPlugin
mashhurs
left a comment
There was a problem hiding this comment.
LGTM for logstash-bridge changes.
Thank you alot for effort!
modules/user-agent/src/main/java/org/elasticsearch/useragent/UserAgentPlugin.java
Outdated
Show resolved
Hide resolved
libs/user-agent-api/src/main/java/org/elasticsearch/useragent/api/UserAgentParsedInfo.java
Outdated
Show resolved
Hide resolved
…elocations * upstream/main: (49 commits) CCS logging fixes (elastic#144070) Improve CPS cluster exclusion handling (elastic#143488) Remove snapshot condition now that node_reduce phase is in non-snapshot builds (elastic#144090) Drop deprecation warnings when updating a mapping in the cluster state applier (elastic#143884) (elastic#144040) Add ensureGreenAndNoInitializingShards helper (elastic#144044) Removed unnecessary applies_to blocks from deprecated query (elastic#144096) [CPS] Use single CrossProjectModeDecider instance (elastic#144030) Fix ESQL TS requests with LIMIT 0 (elastic#144031) ESQL: Remove `create` methods in aggs (elastic#144098) ES|QL: Refactor ChangeLimitOperator (elastic#144017) Add Paginated Hit Source Tests (elastic#142592) Fix test failure not preferred (elastic#144019) Remove serialization logic from EIS authorization response (elastic#144021) ESQL: CSV schema inference and parsing enhancements (elastic#144050) ESQL: Fix incorrectly optimized fork with nullify unmapped_fields (elastic#143030) Fix MMR release test using subqueries (elastic#144087) Refactoring `UserAgentPlugin` (elastic#140712) Drop non-finite samples in Prometheus remote write (elastic#144055) [TEST] Wait for internal inference indices to be created in authorization IT (elastic#143885) Disable ndjson datasource QA tests in release-tests (elastic#143992) ...
Related to #134886
Rationale
Ideally, the user-agent plugin would become a general-purpose plugin that any other plugin/module can use, something like that:
graph BT; UserAgentIngestProcessor --> UserAgentPlugin; UserAgentEsqlCommand --> UserAgentPlugin;With that in mind, it would make the most sense to keep only the user-agent parsing functionality in this plugin (and module), expose it as some kind of a general service, and move the processor to a different module that deals with ingest, for example -
ingest-common.However, the
logstash-bridgeassumes that this plugin is anIngestPluginand this is a very strict API that should remain very stable.Therefore, I decided to keep the processor in this module and keep
UserAgentPluginas an implementation ofIngestPlugin.As a result, most of this PR is related to renaming.
The only functionality change is exposing a user-agent parser registry to the entire node through the plugin dependency injection mechanism. I may also change a bit the user-agent parsing API to accommodate specific ES|QL needs.
Notes for reviewer
logstash-bridgeis not affecteduser_agent.cache_sizethat falls back to the formeringest.user_agent.cache_sizeproperty if not set, that falls back to the same default of 1000.config/ingest-user-agentdirectory as well as a newconfig/user-agentdirectory.Todo list
org.elasticsearch.core.UpdateForV10annotation)