Add support for OpenTelemetry in Glue metastore #18458
Conversation
github-actions
bot
added
tests:hive
iceberg
...-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueOpenTelemetryRequestHandler.java
Outdated
Show resolved
Hide resolved
ebyhr
force-pushed
the
ebi/glue-opentelemetry-v2
branch
from
593fe9b to
718b296
Compare
| install(conditionalModule( | ||
| IcebergGlueCatalogConfig.class, | ||
| IcebergGlueCatalogConfig::isSkipArchive, | ||
| config -> requestHandlers.addBinding().toInstance(new SkipArchiveRequestHandler()))); |
There was a problem hiding this comment.
configis of typeBinder. rename the parameter- i don't think one module should add to set binder created by another module; is it guaranteed to work?
- repeat
newSetBinder(binder, RequestHandler2.class)inside the conditional module
- repeat
There was a problem hiding this comment.
Sent follow-up #18465
Multibinder javadoc mentions:
Contributing multibindings from different modules is supported. For example, it is okay for both CandyModule and ChipsModule to create their own Multibinder, and to each contribute bindings to the set of snacks. When that set is injected, it will contain elements from both modules.
https://google.github.io/guice/api-docs/7.0.0/javadoc/com/google/inject/multibindings/Multibinder.html
There was a problem hiding this comment.
The quoted text says that 2+ modules can call newSetBinder(binder, X.class) and this will end-up being one Set<X>.
it does not seem to say, however, that newSetBinder(binder, X.class) result (the Multibinder object) can be shared between modules
| install(conditionalModule( | ||
| IcebergGlueCatalogConfig.class, | ||
| IcebergGlueCatalogConfig::isSkipArchive, | ||
| config -> requestHandlers.addBinding().toInstance(new SkipArchiveRequestHandler()))); |
| public RequestHandler2 createRequestHandler(OpenTelemetry openTelemetry) | ||
| { | ||
| return AwsSdkTelemetry.builder(openTelemetry) | ||
| .setCaptureExperimentalSpanAttributes(true) |
There was a problem hiding this comment.
nice!
Can we add similar for Iceberg Glue catalog?
There was a problem hiding this comment.
This PR added support for Iceberg Glue catalog as well. The above screenshot is captured with Iceberg connector. GlueMetastoreModule is installed in IcebergGlueCatalogModule.
Description
Extracted from #18368
Release notes
(x) This is not user-visible or docs only and no release notes are required.