Fix/658 structured list properties#673
Conversation
✅ Deploy Preview for otel-ecosystem-explorer ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for “structured list” declarative configuration schemas so certain Java agent config options can be edited as a list of structured objects in the UI.
Changes:
- Extend the
Configurationtype to includedeclarative_typeanddeclarative_schema. - Add a new
StructuredListRendererand route rendering/customization logic to it whendeclarative_type === "structured_list". - Update the inventory/schema mappers to treat
structured_listas a list and provide per-item schema.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ecosystem-explorer/src/types/javaagent.ts | Adds typed fields for declarative_type and declarative_schema on configuration entries. |
| ecosystem-explorer/src/features/java-agent/configuration/components/instrumentation-config-field.tsx | Implements structured-list UI rendering and tweaks type matching to accommodate list values. |
| ecosystem-automation/explorer-db-builder/src/explorer_db_builder/schema_ui_mapper.py | Maps structured_list declarative nodes to list controls and uses declarative_schema for item schema. |
| ecosystem-automation/explorer-db-builder/src/explorer_db_builder/declarative_name_corrections.py | Injects structured_list declarative schema for specific known configuration names. |
| return Array.isArray(value); | ||
| case "map": | ||
| return isPlainObject(value); | ||
| return isPlainObject(value) || Array.isArray(value); |
| {items.map((item, idx) => ( | ||
| <div key={idx} className="border-border/40 space-y-1.5 rounded-md border p-3"> |
| const blank: Record<string, unknown> = {}; | ||
| for (const [key, prop] of properties) { | ||
| blank[key] = prop.type === "boolean" ? (prop.default ?? false) : ""; | ||
| } |
| declarative_schema?: { | ||
| type: "object"; | ||
| required?: string[]; | ||
| properties: Record<string, { type: string; description?: string; default?: unknown }>; |
0db585b to
328d07c
Compare
328d07c to
b41f33c
Compare
|
@jaydeluca pr up for review |
jaydeluca
left a comment
There was a problem hiding this comment.
just a couple comments from myself and copilot to address but this looks good, nice work.
If you are interested, there are a few other places we could update to use this new declarative_type and schema as well, like the instrumentation detail page configuration tab (https://explorer.opentelemetry.io/java-agent/instrumentation/vertx-http-client-4.0) and the config explorer: https://explorer.opentelemetry.io/java-agent/configuration
I can create an issue to handle those in a followup if you'd rather just focus on the current scope of this PR, just figured i'd mention it
| def _classify_node(node: dict[str, Any]) -> str: | ||
| """Classify a schema node into a UI control type.""" | ||
|
|
||
| if node.get("declarative_type") == "structured_list": |
There was a problem hiding this comment.
i could be missing something, but i dont think the changes in this file are needed. I removed it and re-ran the build and everything still works
There was a problem hiding this comment.
Ah, my apologies , you're absolutely right.
I updated schema_ui_mapper.py because I assumed structured_list still needed to be mapped to a standard list control on the backend. I completely overlooked that StructuredListRenderer already handles the declarative type directly on the frontend, so the change wasn't needed at all.
Sorry for the unnecessary noise in the PR. I've reverted the file. Thanks for catching that!
Thanks @jaydeluca! I'm really glad the implementation looks good. I would absolutely love to work on extending this feature to the instrumentation detail page and the config explorer! Please go ahead and create the follow-up issue you can tag or assign me directly to it, and I'll start to work on it as soon as this PR is merged. I've pushed the fixes covering the unit tests and the edge cases Copilot flagged, so this should be ready for another look! Thanks for your time and patience. |
| <div className="w-full max-w-xl space-y-2" aria-label={ariaLabel}> | ||
| {items.map((item, idx) => { | ||
| const stableKey = | ||
| typeof (item as Record<string, unknown>)._id === "string" |
There was a problem hiding this comment.
this _id is leaking into the YAML output now
could we keep _id purely as a render key and not store it in the value? We could Track stable keys with a useRef/WeakMap keyed by item identity. If that ends up being too complex we can just accept index keys (the previous behavior) since these lists are short and append-only.
There was a problem hiding this comment.
Sorry about that! I added _id for stable keys based on a Copilot suggestion, but I didn't realize it would leak into the YAML output.
You're right that idx is sufficient here. I've reverted the change, removed _id, and verified locally that the YAML output is clean now.
Thanks for catching it!
Co-authored-by: Jay DeLuca <jaydeluca4@gmail.com>
|
Thank you for your contribution @DCchoudhury15! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
What changed
Added support for structured list configurations in the declarative config builder.
Backend (
explorer-db-builder)java.common.service_peer_mappingurl_template_rulesstructured_listconfigs and expose the injected schema as the list item schemaFrontend (
ecosystem-explorer)declarative_typeanddeclarative_schemaStructuredListRendererfor rendering structured list entriesWhy
Fixes #658.
service_peer_mappingandurl_template_rulesare structured configurations, but were previously rendered as generic key/value maps. This made it unclear which fields were expected and resulted in an inconsistent editing experience.This change allows these configurations to be rendered using their schema definition, producing a more intuitive UI and correct YAML output.
Screenshot
Testing
Manual
java.common.service_peer_mappingin the config builder