Skip to content

Expose orchestrator roles #3201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
sitole wants to merge 1 commit into
base: main
Choose a base branch
from
+1 −0
Draft

Expose orchestrator roles #3201

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/orchestrator/pkg/factories/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@
version,
serviceInstanceID,
attribute.Key("host.labels").StringSlice(config.NodeLabels),
attribute.Key("service.roles").StringSlice(config.Services),

Check warning on line 306 in packages/orchestrator/pkg/factories/run.go

View check run for this annotation

Claude / Claude Code Review

service.roles uses unnormalized config.Services 

The new `service.roles` attribute uses the raw `config.Services` slice, but everywhere else in `run()` uses the normalized `services := cfg.GetServices(config)` (which lowercases, trims whitespace, and drops unknown entries). This means `service.roles` can emit values like ` orchestrator `, `TEMPLATE-MANAGER`, or a typo'd `unknown` that never appear in `service.name` — undermining the stated goal of easy filter-then-unify across variants. Fix: build a `[]string` from the already-parsed `services

@cursor cursor Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Roles skip GetServices normalization

Medium Severity

service.roles is set from raw config.Services while service.name and runtime behavior use cfg.GetServices(config) on the same line 236. Unknown entries, casing, and whitespace can appear in OTEL but not in what actually runs, so role-based OTEL filters can miss or mislabel instances.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a0547d5. Configure here.

@sitole sitole Jul 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

expected

@claude claude Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The new service.roles attribute uses the raw config.Services slice, but everywhere else in run() uses the normalized services := cfg.GetServices(config) (which lowercases, trims whitespace, and drops unknown entries). This means service.roles can emit values like orchestrator, TEMPLATE-MANAGER, or a typo'd unknown that never appear in service.name — undermining the stated goal of easy filter-then-unify across variants. Fix: build a []string from the already-parsed services slice (e.g. slices.Collect(func(yield ...) { for _, s := range services { yield(string(s)) } }), or a simple loop) and pass that.

Extended reasoning...

What the bug is. In packages/orchestrator/pkg/factories/run.go, the new telemetry attribute added at line 306 is attribute.Key("service.roles").StringSlice(config.Services). config.Services is the raw []string parsed from the ORCHESTRATOR_SERVICES env var (see packages/orchestrator/pkg/cfg/model.go), with no normalization. Everywhere else in run() uses the local variable services := cfg.GetServices(config), which calls ParseServiceType in packages/orchestrator/pkg/cfg/service.go — that helper lowercases input, trims whitespace, and drops entries it can't map (returning UnknownService, which GetServices filters out).\n\nCode path that triggers it. serviceName := cfg.GetServiceName(services) on the parsed slice produces canonical service.name values like orchestrator, template-manager, or orchestrator_template-manager. But the new service.roles reads directly from config.Services, so the two attributes are computed from different sources — one normalized, one raw. The service_test.go:63 test explicitly exercises this normalization on inputs like " orchestrator ", "unknown", "TEMPLATE-MANAGER".\n\nWhy existing code doesn't prevent it. There is no validation between config parsing and telemetry attribute construction. Config.Services is used as-is — the parse step in GetServices is called once locally and its result never written back to config.\n\nStep-by-step proof. Suppose an operator sets ORCHESTRATOR_SERVICES=" orchestrator ,TEMPLATE-MANAGER,unknown".\n1. Config parsing splits on , and produces config.Services = [" orchestrator ", "TEMPLATE-MANAGER", "unknown"].\n2. services := cfg.GetServices(config) returns [Orchestrator, TemplateManager] (trimmed, lowercased, unknown dropped).\n3. serviceName := cfg.GetServiceName(services) produces the canonical "orchestrator_template-manager".\n4. But attribute.Key("service.roles").StringSlice(config.Services) emits [" orchestrator ", "TEMPLATE-MANAGER", "unknown"] verbatim.\n5. A Grafana filter of service.roles = "orchestrator" would miss this host; service.name and service.roles are computed from different domains and cannot be cross-filtered reliably.\n\nImpact. Practically low — the production deployment configs in iac/provider-{gcp,aws} and .github/actions/start-services/action.yml pass clean canonical values, so no current host would diverge. But the stated PR goal ("easily filter … and later switch to a unified service name") depends on canonical tag values, so the divergence is a latent inconsistency that will bite as soon as a config drifts.\n\nHow to fix. Convert the already-parsed services slice into a []string and pass that:\n\ngo\nroles := make([]string, len(services))\nfor i, s := range services {\n roles[i] = string(s)\n}\n// ...\nattribute.Key("service.roles").StringSlice(roles),\n\n\nThat guarantees service.roles values are drawn from the same canonical vocabulary as service.name.

)
if err != nil {
logger.L().Fatal(ctx, "failed to init telemetry", zap.Error(err))
Expand Down
Loading