Entity SDK - Initial opt-in SDK features#8464
Conversation
- Create new Entity Detectors that are completely separate from existing reosurce detectors so entity must be FULL opt-in before behavior changes, even if OTLP is non-breaking. - Wire "either/or" scenarios into resource factory and declarative config. TODOs we need to sort out: - Features of ResourceConfiguration spec that interact poorly with entities, e.g. attribute-specific filters that aren't entity aware, raw attribute definitions. - Adding `env` as a supported detector for reading in Entity env variable propagation. - Dealing with oddities of ResourceBuilder/Factory wanting to only use attributes in Java - we worked around it for now.
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
You'll need to handle some merge conflicts since declarative config has been undergoing construction:
- It moved out of the incubator into a dedicated artifact in #8265
- We started commiting generated POJOs to VCS in #8408
As mentioned in the SIG meeting, I don't think you need the incubator copies of Entity interfaces. I pushed a commit here to delete them: jack-berg@3ea17f4
Per the discussion, resource detectors will need to updated to optionally produce entity-aware resources. If the user opts in, then those resource detectors can call the internal APIs from opentelemetry-sdk-common to produce those.
| * | ||
| * @return a map of attributes. | ||
| */ | ||
| abstract Attributes getRawAttributes(); |
There was a problem hiding this comment.
These are loose attributes, i.e. those that weren't attached to an Enttiy. We need this for backwards compatibility
There was a problem hiding this comment.
Now that you're memoizing during initialization, is this still needed? I see exactly one usage in ResourceBuilder.putAll(Resource). I suppose keeping this preserves the rawness of the attributes (as opposed to being associated with an entity) during merging, but is there any functional difference from just changing:
To:
/** Puts all attributes from {@link Resource} into this. */
public ResourceBuilder putAll(Resource resource) {
if (resource != null) {
// Preserve entities when merging resources.
entities.addAll(resource.getEntities());
attributesBuilder.putAll(resource.getAttributes());
}
return this;
}
Or better yet, compute the raw attributes during putAll:
/** Puts all attributes from {@link Resource} into this. */
public ResourceBuilder putAll(Resource resource) {
if (resource != null) {
// Preserve entities when merging resources.
entities.addAll(resource.getEntities());
AttributesBuilder rawAttributes = resource.getAttributes().toBuilder();
rawAttributes.removeIf(
key ->
resource.getEntities().stream()
.anyMatch(
entity ->
entity.getId().get(key) != null
|| entity.getDescription().get(key) != null));
attributesBuilder.putAll(rawAttributes.build());
}
return this;
}
Or better better yet (😛), extract a dedicated static function for computing the raw attributes for a resource (since I did end up finding one more use of raw attributes in EntityUtil):
public static Attributes getRawAttributes(Resource resource) {
AttributesBuilder rawAttributes = resource.getAttributes().toBuilder();
rawAttributes.removeIf(
key ->
resource.getEntities().stream()
.anyMatch(
entity ->
entity.getId().get(key) != null
|| entity.getDescription().get(key) != null));
return rawAttributes.build();
}
There was a problem hiding this comment.
I'm trying to keep Resource, as best we can, match the actual Resource data model in java -
<- Here you have entities + "loose" attributes as the data model.
I've updated Resource to expose the "raw" attribute getter method internally and compute it as you suggest, but I still want, conceptually, for this to exist on Resource to make the data model more clearly represented.
There was a problem hiding this comment.
Are you saying that "loose attributes" / "raw attributes" are the same thing as the attributes field from this table? I.e. "Additional Attributes that identify the resource."
If no, then I think loose / raw attributes should appear somewhere in that table if we're going to formally model them.
If yes, then that's fine, but I think it would be good to connect the dots and include the word "loose" or raw' in the description of "Attributes" field.
There was a problem hiding this comment.
The answer is yes. Ideally in java we'd just have attributes be used as well to match the data model. The issue is getAttributes is already taken to denote the entire collapsed set, so I needed a new term to represent attributes as described in that table.
I think it's fair for us to add loose attributes as an APi field - I think that would be here.
We are using this PR as the foundation of SDK changes - so I can draft a PR to the SDK appropriately for entities now so you can see this all line up and we can discuss naming on the spec side.
There was a problem hiding this comment.
FYI - let the name bikeshedding begin - open-telemetry/opentelemetry-specification#5201
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8464 +/- ##
============================================
- Coverage 91.62% 91.61% -0.02%
- Complexity 10321 10390 +69
============================================
Files 1013 1021 +8
Lines 27287 27620 +333
Branches 3203 3252 +49
============================================
+ Hits 25001 25303 +302
- Misses 1559 1564 +5
- Partials 727 753 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Simplify API for using entities. - Update so we can use current resource detection unchanged with entities. - Memoize full attributes manually when creating a resource.
jsuereth
changed the title
| * | ||
| * @return a map of attributes. | ||
| */ | ||
| abstract Attributes getRawAttributes(); |
There was a problem hiding this comment.
Now that you're memoizing during initialization, is this still needed? I see exactly one usage in ResourceBuilder.putAll(Resource). I suppose keeping this preserves the rawness of the attributes (as opposed to being associated with an entity) during merging, but is there any functional difference from just changing:
To:
/** Puts all attributes from {@link Resource} into this. */
public ResourceBuilder putAll(Resource resource) {
if (resource != null) {
// Preserve entities when merging resources.
entities.addAll(resource.getEntities());
attributesBuilder.putAll(resource.getAttributes());
}
return this;
}
Or better yet, compute the raw attributes during putAll:
/** Puts all attributes from {@link Resource} into this. */
public ResourceBuilder putAll(Resource resource) {
if (resource != null) {
// Preserve entities when merging resources.
entities.addAll(resource.getEntities());
AttributesBuilder rawAttributes = resource.getAttributes().toBuilder();
rawAttributes.removeIf(
key ->
resource.getEntities().stream()
.anyMatch(
entity ->
entity.getId().get(key) != null
|| entity.getDescription().get(key) != null));
attributesBuilder.putAll(rawAttributes.build());
}
return this;
}
Or better better yet (😛), extract a dedicated static function for computing the raw attributes for a resource (since I did end up finding one more use of raw attributes in EntityUtil):
public static Attributes getRawAttributes(Resource resource) {
AttributesBuilder rawAttributes = resource.getAttributes().toBuilder();
rawAttributes.removeIf(
key ->
resource.getEntities().stream()
.anyMatch(
entity ->
entity.getId().get(key) != null
|| entity.getDescription().get(key) != null));
return rawAttributes.build();
}
| @SuppressWarnings("JdkObsolete") // Recommended alternative was introduced in java 10 | ||
| static Resource createEnvironmentResource(ConfigProperties config) { | ||
| boolean entitiesEnabled = | ||
| config.getBoolean(EntityExperimentConstants.EXPERIMENTAL_ENTITIES_ENABLED, false); |
There was a problem hiding this comment.
This is an interesting way to get around having OTEL_ENTITIES and not OTEL_EXPERIMENTAL_ENTITIES or something similarly qualified as experimental. Allows us to use the ultimate target env var OTEL_ENTITIES while retaining the ability to change the semantics since users have to opt in.
No precedent for this type of thing but I think its fine. WDYT @open-telemetry/java-approvers ?
|
This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome. For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question. Automation flags a PR for human review once every review thread has a reply or is marked as resolved. Status across open PRs is visible on the pull request dashboard. |
QT_ACCESSIBILITY=1 COLORTERM=truecolor HISTCONTROL=ignoredups XDG_MENU_PREFIX=gnome- TERM_PROGRAM_VERSION=1.107.0 GNOME_DESKTOP_SESSION_ID=this-is-deprecated GTK_IM_MODULE=ibus QT_IM_MODULES=wayland;ibus PULSE_RUNTIME_PATH=/run/user/110055/crd_audio#_Bm5JrFQNq CHROME_REMOTE_DESKTOP_HOST_EXTRA_PARAMS=--enable-utempter P4CONFIG=.p4config SSH_AUTH_SOCK=/run/user/110055/crd_ssh_auth_sock MEMORY_PRESSURE_WRITE=c29tZSAyMDAwMDAgMjAwMDAwMAA= XMODIFIERS=@im=ibus X20_HOME=/google/data/rw/users/jo/joshuasuereth ANTIGRAVITY_CLI_ALIAS=jetski-ide GTK_MODULES=gail:atk-bridge PWD=/usr/local/google/home/joshuasuereth/projects/open-telemetry/opentelemetry-java RSYNC_RSH=ssh LOGNAME=joshuasuereth XDG_SESSION_TYPE=x11 CHROME_CONFIG_HOME=/usr/local/google/home/joshuasuereth/.config/chrome-remote-desktop/chrome-config GPG_AGENT_INFO=/run/user/110055/gnupg/S.gpg-agent:0:1 SYSTEMD_EXEC_PID=4924 XAUTHORITY=/usr/local/google/home/joshuasuereth/.Xauthority VSCODE_GIT_ASKPASS_NODE=/opt/jetski-ide/jetski GOOGLE_API_CERTIFICATE_CONFIG=/etc/gcloud/certificate_config.json P4MERGE=/google/src/files/head/depot/eng/perforce/mergep4.tcl PULSE_SINK=chrome_remote_desktop_session HOME=/usr/local/google/home/joshuasuereth LANG=en_US.UTF-8 CLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE=true LS_COLORS=rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=00:su=37;41:sg=30;43:ca=00:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.7z=01;31:*.ace=01;31:*.alz=01;31:*.apk=01;31:*.arc=01;31:*.arj=01;31:*.bz=01;31:*.bz2=01;31:*.cab=01;31:*.cpio=01;31:*.crate=01;31:*.deb=01;31:*.drpm=01;31:*.dwm=01;31:*.dz=01;31:*.ear=01;31:*.egg=01;31:*.esd=01;31:*.gz=01;31:*.jar=01;31:*.lha=01;31:*.lrz=01;31:*.lz=01;31:*.lz4=01;31:*.lzh=01;31:*.lzma=01;31:*.lzo=01;31:*.pyz=01;31:*.rar=01;31:*.rpm=01;31:*.rz=01;31:*.sar=01;31:*.swm=01;31:*.t7z=01;31:*.tar=01;31:*.taz=01;31:*.tbz=01;31:*.tbz2=01;31:*.tgz=01;31:*.tlz=01;31:*.txz=01;31:*.tz=01;31:*.tzo=01;31:*.tzst=01;31:*.udeb=01;31:*.war=01;31:*.whl=01;31:*.wim=01;31:*.xz=01;31:*.z=01;31:*.zip=01;31:*.zoo=01;31:*.zst=01;31:*.avif=01;35:*.jpg=01;35:*.jpeg=01;35:*.jxl=01;35:*.mjpg=01;35:*.mjpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.webm=01;35:*.webp=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=00;36:*.au=00;36:*.flac=00;36:*.m4a=00;36:*.mid=00;36:*.midi=00;36:*.mka=00;36:*.mp3=00;36:*.mpc=00;36:*.ogg=00;36:*.ra=00;36:*.wav=00;36:*.oga=00;36:*.opus=00;36:*.spx=00;36:*.xspf=00;36:*~=00;90:*#=00;90:*.bak=00;90:*.crdownload=00;90:*.dpkg-dist=00;90:*.dpkg-new=00;90:*.dpkg-old=00;90:*.dpkg-tmp=00;90:*.old=00;90:*.orig=00;90:*.part=00;90:*.rej=00;90:*.rpmnew=00;90:*.rpmorig=00;90:*.rpmsave=00;90:*.swp=00;90:*.tmp=00;90:*.ucf-dist=00;90:*.ucf-new=00;90:*.ucf-old=00;90: XDG_CURRENT_DESKTOP=GNOME PYTHONSTARTUP=/usr/local/google/home/joshuasuereth/.config/Jetski/User/workspaceStorage/cbb7ec0eec555b4d06671311926a9b6f/ms-python.python/pythonrc.py MEMORY_PRESSURE_WATCH=/sys/fs/cgroup/user.slice/user-110055.slice/user@110055.service/session.slice/org.gnome.Shell@x11.service/memory.pressure VTE_VERSION=8390 VSCODE_JAVA_EXEC=/usr/lib/jvm/java-21-openjdk-amd64/bin/java GIT_ASKPASS=/opt/jetski-ide/resources/app/extensions/git/dist/askpass.sh GNOME_TERMINAL_SCREEN=/org/gnome/Terminal/screen/1538e342_19f1_4bdb_9b1d_20e770cebc04 STREAMZ_SERVERS=[2001:4860:f802::78]:9530 CHROME_DESKTOP=jetski.desktop CLUTTER_IM_MODULE=ibus INFOPATH=/usr/share/emacs-google-config/info: CHROME_REMOTE_DESKTOP_SESSION=1 VSCODE_GIT_ASKPASS_EXTRA_ARGS= VSCODE_PYTHON_AUTOACTIVATE_GUARD=1 CLOUDSDK_CONTAINER_USE_APPLICATION_DEFAULT_CREDENTIALS=true LESSCLOSE=/usr/bin/lesspipe %s %s XDG_SESSION_CLASS=user PYTHONPATH=/usr/local/buildtools/current/sitecustomize TERM=xterm-256color PYTHON_BASIC_REPL=1 LESSOPEN=| /usr/bin/lesspipe %s SK_SIGNING_PLUGIN=gnubbyagent USER=joshuasuereth VSCODE_GIT_IPC_HANDLE=/run/user/110055/vscode-git-e5a8f99bf6.sock GNOME_TERMINAL_SERVICE=:1.94 CHROME_REMOTE_DESKTOP_DEFAULT_DESKTOP_SIZES=1600x1200,3840x2160,3840x2560,5120x1440,2160x3840 CLOUDSDK_CONTEXT_AWARE_USE_MTLS_FOR_GRPC=true DISPLAY=:20 SHLVL=1 PARINIT=rTbgqR B=.?_A_a Q=_s>|: CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH=/etc/gcloud/certificate_config.json GOOGLE_AUTH_WEBAUTHN_PLUGIN=gcloudwebauthn QT_IM_MODULE=ibus CVS_RSH=ssh MANAGERPIDFDID=2490 FC_FONTATIONS=1 LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu:/usr/lib/x86_64-linux-gnu/mesa:/usr/lib/x86_64-linux-gnu/dri:/usr/lib/x86_64-linux-gnu/gallium-pipe XDG_RUNTIME_DIR=/run/user/110055 PIPEWIRE_REMOTE=crd_audio#_Bm5JrFQNq/pipewire VSCODE_GIT_ASKPASS_MAIN=/opt/jetski-ide/resources/app/extensions/git/dist/askpass-main.js GTK3_MODULES=xapp-gtk3-module VSCODE_JDWP_ADAPTER_ENDPOINTS=/usr/local/google/home/joshuasuereth/.jetski/extensions/vscjava.vscode-java-debug-0.59.0-universal/.noConfigDebugAdapterEndpoints/endpoint-c1594771e7136dbf.txt XDG_DATA_DIRS=/usr/share/gnome:/usr/local/share/:/usr/share/ GDK_BACKEND=x11 PATH=/usr/local/google/home/joshuasuereth/.local/bin:/usr/local/google/home/joshuasuereth/bin:/usr/local/google/home/joshuasuereth/.cargo/bin:/usr/local/google/home/joshuasuereth/.local/bin:/usr/local/google/home/joshuasuereth/bin:/usr/lib/google-golang/bin:/usr/local/buildtools/java/jdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/google/home/joshuasuereth/.jetski/extensions/vscjava.vscode-java-debug-0.59.0-universal/bundled/scripts/noConfigScripts GOOGLE_CLOUD_DISABLE_DIRECT_PATH=true CLOUDSDK_INTERNAL_USER=true DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/110055/bus CLOUDSDK_CONTEXT_AWARE_USE_ECP_HTTP_PROXY=true DATA_CLOUD_CURR_IDE_NAME=jetski TERM_PROGRAM=vscode BASH_FUNC_poetry%%=() { /usr/bin/gpkg poetry "$@" } BASH_FUNC_yarn%%=() { /usr/bin/gpkg yarn "$@" } BASH_FUNC_pip%%=() { /usr/bin/gpkg pip "$@" } BASH_FUNC_pdm%%=() { /usr/bin/gpkg pdm "$@" } BASH_FUNC_uvx%%=() { /usr/bin/gpkg uvx "$@" } BASH_FUNC_npm%%=() { /usr/bin/gpkg npm "$@" } BASH_FUNC_npx%%=() { /usr/bin/gpkg npx "$@" } BASH_FUNC_pip3%%=() { /usr/bin/gpkg pip3 "$@" } BASH_FUNC_python3%%=() { /usr/bin/gpkg python3 "$@" } BASH_FUNC_pipx%%=() { /usr/bin/gpkg pipx "$@" } BASH_FUNC_pnpx%%=() { /usr/bin/gpkg pnpx "$@" } BASH_FUNC_pnpm%%=() { /usr/bin/gpkg pnpm "$@" } BASH_FUNC_yarnpkg%%=() { /usr/bin/gpkg yarnpkg "$@" } BASH_FUNC_uv%%=() { /usr/bin/gpkg uv "$@" } BASH_FUNC_python%%=() { /usr/bin/gpkg python "$@" } BASH_FUNC_hatch%%=() { /usr/bin/gpkg hatch "$@" } _=/usr/bin/env resource providerr.
|
PR with my remaining feedback here: jsuereth#1 |
Feedback for entities impl
|
@jack-berg TWo questions from the PR - but one important you should decide on before merging here - Do you want to rename "Raw Attributes" to "Unassociated Attributes"? I realize it's semi-private API but it's simple enough to change. |
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Looks good from my side. I don't have any preference on "raw attributes" vs "unassociated attributes" naming. Choose whatever is most likely to pass spec scrutiny, but we can merge here even without spec and update our naming later if needed.
Still need to merge main and fix some minor things.
Also, should update the PR description to reflect the final state, so its accurate for any future readers.
|
@jack-berg I updated the method names and hopefully have tests passing (ran spotless locally) |
2 participants
Adds initial Entity support to OpenTelemetry Java SDK.
What this does:
otel.exprimental.entities.enabledthat will turn on Entity capabilities. This can be found inio.opentelemetry.sdk.autoconfigure.spi.internal.EntityExperimentConstants.EnvironmentResourceto parseOTEL_ENTITIESenv variable. Note: This includes a full parser.env-named Resource Detector which delegates toEnvironmentResource, but always enables entity flag for that detection.service-named ResourceDetector to support entities when experiment flag is set to true.