Skip to content

chore: telemetry updates for the deployments#11500

Merged
jvillafanez merged 3 commits intomasterfrom
deployments_telemetry_updates
Jul 16, 2025
Merged

chore: telemetry updates for the deployments#11500
jvillafanez merged 3 commits intomasterfrom
deployments_telemetry_updates

Conversation

@jvillafanez
Copy link
Member

Keycloak example will be updated to 26.2.5, which contains support for telemetry. The committed setup changes are required for the Keycloak update.

Due to the Keycloak update to 26.2.5, jaeger will also need to be updated. It isn't part of the deployment, but the expected docker image is jaegertracing/jaeger:2.7.0 (which is supported by Keycloak 26.2.5).

The jaeger update also brings changes in oCIS. Previous 6831 port in jaeger isn't available in recent versions (particularly 2.7.0), and has changed to port 4317, which supports the "otlp" tracing type in oCIS. No code change is needed.

Description

Update setup for telemetry. Note that there are references to jaeger:4317, but such container isn't provided (as it wasn't before this PR). You can use the container below if needed (adjust what you need)

  jaeger:
    image: jaegertracing/jaeger:2.7.0
    networks:
      ocis-net:
    ports:
      - "16686:16686"
    environment:
      COLLECTOR_ZIPKIN_HTTP_PORT: 9411
    restart: always

Related Issue

#11298

Motivation and Context

How Has This Been Tested?

Manually tested with the jaeger container shown above. There are traces for keycloak and ocis services in the jaeger instance.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Keycloak example will be updated to 26.2.5, which contains support for
telemetry. The committed setup changes are required for the Keycloak
update.

Due to the Keycloak update to 26.2.5, jaeger will also need to be
updated. It isn't part of the deployment, but the expected docker image
is jaegertracing/jaeger:2.7.0 (which is supported by Keycloak 26.2.5).

The jaeger update also brings changes in oCIS. Previous 6831 port in
jaeger isn't available in recent versions (particularly 2.7.0), and has
changed to port 4317, which supports the "otlp" tracing type in oCIS. No
code change is needed.
@jvillafanez jvillafanez self-assigned this Jul 4, 2025
@update-docs
Copy link

update-docs bot commented Jul 4, 2025

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@mmattel
Copy link
Contributor

mmattel commented Jul 4, 2025

@jvillafanez Q: we have many locations in the ocis code that have in the envvar text description jaeger instead (or additional) of oltp, see example screenshot:
Shouldnt this be changed/updated too?
In case, this would need an additional PR and a changelog that jaeger got replaced or oltp got added...
image

@jvillafanez
Copy link
Member Author

It seems it got added in #5132 , but I don't know if it's completed or not. At least "it works for me".
We can add "otlp" as supported. We might want to eventually remove "jaeger" as supported type and stick with "otlp", but maybe is still soon. From a code perspective, there is no change in the behavior, and the default is "jaeger"

@mmattel
Copy link
Contributor

mmattel commented Jul 4, 2025

We can add "otlp" as supported.

Seems to me as good and consistent approach.

@2403905
Copy link
Contributor

2403905 commented Jul 7, 2025

@jvillafanez Should we switch the ocis to the otlp by default?

@jvillafanez
Copy link
Member Author

We'll probably need to switch the default at some point, but I'd prefer to ensure it works without problems. It can wait for a major version.

@jvillafanez
Copy link
Member Author

The jaeger update also brings changes in oCIS. Previous 6831 port in jaeger isn't available in recent versions (particularly 2.7.0), and has changed to port 4317, which supports the "otlp" tracing type in oCIS

For reference: https://www.jaegertracing.io/docs/2.7/architecture/apis/ port 6831 is deprecated. It might still be possible to run it but it might not be trivial, and the port isn't running by default.

@mmattel
Copy link
Contributor

mmattel commented Jul 14, 2025

Could you add otlp to the allowed envvar OCIS_TRACING_TYPE where used as discussed above?

@jvillafanez jvillafanez requested a review from LukasHirt as a code owner July 14, 2025 12:35
type Config struct {
Enabled bool `yaml:"enabled" env:"OCIS_TRACING_ENABLED" desc:"Activates tracing." introductionVersion:"pre5.0"`
Type string `yaml:"type" env:"OCIS_TRACING_TYPE" desc:"The type of tracing. Defaults to \"\", which is the same as \"jaeger\". Allowed tracing types are \"jaeger\" and \"\" as of now." introductionVersion:"pre5.0" introductionVersion:"pre5.0"`
Type string `yaml:"type" env:"OCIS_TRACING_TYPE" desc:"The type of tracing. Defaults to \"\", which is the same as \"jaeger\". Allowed tracing types are \"jaeger\", \"otlp\" and \"\" as of now." introductionVersion:"pre5.0" introductionVersion:"pre5.0"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Type string `yaml:"type" env:"OCIS_TRACING_TYPE" desc:"The type of tracing. Defaults to \"\", which is the same as \"jaeger\". Allowed tracing types are \"jaeger\", \"otlp\" and \"\" as of now." introductionVersion:"pre5.0" introductionVersion:"pre5.0"`
Type string `yaml:"type" env:"OCIS_TRACING_TYPE" desc:"The type of tracing. Defaults to '', which is the same as 'jaeger'. Allowed tracing types are 'jaeger', 'otlp' and '' as of now." introductionVersion:"pre5.0" introductionVersion:"pre5.0"`

For the sake that we everywhere else use single quotes instead of double quotes. This for sure was just a leftover from ancient times 😅

This is just for documentation purposes. There is no code change.
@jvillafanez jvillafanez force-pushed the deployments_telemetry_updates branch from f52e748 to 641fa42 Compare July 14, 2025 13:29
@jvillafanez
Copy link
Member Author

I think we can skip the "tag alignment" error in sonarCloud. Not sure why it only triggers there when it should happen in every config file. We'll probably need to reformat a lot of files.

@mmattel
Copy link
Contributor

mmattel commented Jul 14, 2025

I think we can skip the "tag alignment" error in sonarCloud.

How can this then be merged?

@mmattel
Copy link
Contributor

mmattel commented Jul 15, 2025

@jvillafanez hmmm, if I compare:

proxy:
Type string yaml:"type" env:"OCIS_TRACING_TYPE;PROXY_TRACING_TYPE" desc:"The type of tracing. Defaults to '', which is the same as 'jaeger'. Allowed tracing types are 'jaeger', 'otlp' and '' as of now." introductionVersion:"pre5.0"

and

search:
Type string ocisConfig:"type" env:"OCIS_TRACING_TYPE;SEARCH_TRACING_TYPE" desc:"The type of tracing. Defaults to '', which is the same as 'jaeger'. Allowed tracing types are 'jaeger', 'otlp' and '' as of now." introductionVersion:"pre5.0"

why do we have in search --> ocisConfig instead of yaml ?
all others have yaml.
maybe this is the reason sonarcloud complains...

@jvillafanez
Copy link
Member Author

As far as I know, sonarcloud's proposed solution is to move the "ocisConfig" to the end of the line, which doesn't make sense. That's why I'm confused.

Regarding the "ocisConfig" vs "yaml", #3412 is what I've found. I guess it's kind of dead code or code that doesn't work any longer. I assume those vars won't be loaded from the yaml file (which seems wrong to me), but they should be loaded from the environment variables.
It's probably something they forgot to update, or was copied from another service previous to that PR.
I'll add the change here to see if it fixes the problem with sonarcloud.

@sonarqubecloud
Copy link

@jvillafanez
Copy link
Member Author

it seems it worked 🤷

@mmattel
Copy link
Contributor

mmattel commented Jul 16, 2025

merging?

@jvillafanez jvillafanez merged commit 2f0842e into master Jul 16, 2025
4 checks passed
@jvillafanez jvillafanez deleted the deployments_telemetry_updates branch July 16, 2025 11:26
ownclouders pushed a commit that referenced this pull request Jul 16, 2025
chore: telemetry updates for the deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants