Skip to content

Kanban 1855: Add SecretProvider.getAppName() for per-app cost attribution#7867

Merged
labkey-gokhano merged 3 commits into
developfrom
fb_gemini_vertex_cost_labels
Jul 22, 2026
Merged

Kanban 1855: Add SecretProvider.getAppName() for per-app cost attribution#7867
labkey-gokhano merged 3 commits into
developfrom
fb_gemini_vertex_cost_labels

Conversation

@labkey-gokhano

Copy link
Copy Markdown
Contributor

Rationale

premiumModules' SsmSecretProvider needs to report which application it's scoped to, so McpServiceImpl can attribute Vertex AI request cost per app (via GCP billing labels) without knowing anything about SSM, the secrets prefix, or its format. Rather than sharing a raw string constant across a module boundary, SecretProvider gains a proper SPI method for this, following the same pattern as getSecret()/getDescription().

Kanban epic: https://github.com/LabKey/kanban/issues/1855

Related Pull Requests

  • LabKey/premiumModules#665 -- consumes this via SsmSecretProvider.getAppName() to label Vertex AI requests for per-app cost attribution

Changes

  • SecretProvider.getAppName() -- new default method, returns null unless an implementation overrides it. Non-breaking for all existing providers.
  • SecretService.getAppName() -- queries active providers in priority order, returns the first non-null app name.
  • SecretServiceImpl -- implementation plus two new unit tests (provider reports a name; no active provider reports one).

…tion

SecretProvider gains a default getAppName() (null by default), and SecretService exposes it by querying active providers in priority order. This gives any SecretProvider implementation a standard way to report the application it's scoped to, without callers needing to know which concrete provider is active or how it derives that name.

Consumed by premiumModules' SsmSecretProvider (LabKey/premiumModules#665) to attribute Vertex AI request cost per app via GCP billing labels, without professional needing a direct dependency on cloudServices.
@labkey-matthewb

Copy link
Copy Markdown
Contributor

I can see why this seems like an easy way to wire this up because currently the use of AppName "goes with" the use of the Gemini key. However, this looks like a pretty general purpose property. As a dev, I'd probably look for this first on AppProps.getInstance(). If we know the app-name when we create the instance could this be loaded as another property in application.properties?

@labkey-adam

Copy link
Copy Markdown
Contributor

I can see why this seems like an easy way to wire this up because currently the use of AppName "goes with" the use of the Gemini key. However, this looks like a pretty general purpose property. As a dev, I'd probably look for this first on AppProps.getInstance(). If we know the app-name when we create the instance could this be loaded as another property in application.properties?

I agree we have a multitude of ways we could provide an "app name." And that app name isn't inherently tied up with secrets. I'm okay with taking this approach for now, given that the app name is already being "set" and doesn't require a new startup property or application.properties changes by syseng. Or course, it would be easy to add an AppProps method that delegates (for now) to the SecretService and could change to some more general implementation in the future.

@labkey-matthewb

Copy link
Copy Markdown
Contributor

I agree this is workable for now. It's just backward from how I imagined app name being used. I thought this would be a link/key the server would use to configure itself (I'm labkey_org how should I set myself up) rather than a "secret" that is handed in. You can imagine pushing much less configuration stuff onto the image and asking for it at startup.

@labkey-gokhano

Copy link
Copy Markdown
Contributor Author

@labkey-matthewb @labkey-adam How about this proposal to bridge the two concerns here:

  1. keep SecretService.getAppName() as the actual source of truth (no new syseng/DevOps-managed property, per Adam's point),
  2. but also expose it as a default method on AppProps that delegates to it, so it lives where a dev would naturally look first (Matthew's point).

Concretely, in AppProps.java, right next to getEnlistmentId() since both are optional identity strings about the deployment:

/**

  • Returns the DevOps-assigned name of the tenant/customer this deployment belongs to (e.g., "Hooli",
  • "WNPRC"), or {@code null} if none is configured. Not customer-facing; currently sourced from
  • whichever active SecretProvider reports one.
  • @see SecretService#getAppName()
    */
    default @nullable String getAppName()
    {
    return SecretService.get().getAppName();
    }

Since it's a default method, AppPropsImpl needs no changes which makes this purely a discoverability layer over the existing mechanism, not a second implementation or a new config source.

Matthew, this doesn't address your bigger point about the server asking for its own identity at startup rather than having it handed in. That is outside the scope of this PR but I am happy to open a follow-up/epic for that if you think it's worth pursuing separately.

Please let me know if this works for you both, or if you'd rather keep app name off AppProps entirely for now.

@labkey-adam

Copy link
Copy Markdown
Contributor

@labkey-matthewb @labkey-adam How about this proposal to bridge the two concerns here:

  1. keep SecretService.getAppName() as the actual source of truth (no new syseng/DevOps-managed property, per Adam's point),
  2. but also expose it as a default method on AppProps that delegates to it, so it lives where a dev would naturally look first (Matthew's point).

Concretely, in AppProps.java, right next to getEnlistmentId() since both are optional identity strings about the deployment:

/**

  • Returns the DevOps-assigned name of the tenant/customer this deployment belongs to (e.g., "Hooli",
  • "WNPRC"), or {@code null} if none is configured. Not customer-facing; currently sourced from
  • whichever active SecretProvider reports one.
  • @see SecretService#getAppName()
    */
    default @nullable String getAppName()
    {
    return SecretService.get().getAppName();
    }

Since it's a default method, AppPropsImpl needs no changes which makes this purely a discoverability layer over the existing mechanism, not a second implementation or a new config source.

Matthew, this doesn't address your bigger point about the server asking for its own identity at startup rather than having it handed in. That is outside the scope of this PR but I am happy to open a follow-up/epic for that if you think it's worth pursuing separately.

Please let me know if this works for you both, or if you'd rather keep app name off AppProps entirely for now.

Yep, this is what I suggested above, and I'm on board. McpServiceImpl should call the AppProps method to retrieve the app name.

@labkey-gokhano

labkey-gokhano commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Update: Adam's on board with this. Pushed the AppProps.getAppName() addition, and also updated McpServiceImpl (LabKey/premiumModules#665) to call AppProps.getInstance().getAppName() instead of SecretService.get().getAppName() directly, so the consumer goes through the same discoverable entry point.

Exposes app name where a dev would naturally look first (on AppProps), while keeping SecretService as the actual source of truth so no new syseng/DevOps-managed startup property is needed. Addresses PR review feedback from Matthew and Adam on #7867.
@labkey-gokhano
labkey-gokhano merged commit b3d6a9a into develop Jul 22, 2026
7 checks passed
@labkey-gokhano
labkey-gokhano deleted the fb_gemini_vertex_cost_labels branch July 22, 2026 01:05
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.

4 participants