Skip to content

Remove advisor low-cardinality keys that aren't actually exposed. #1660

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.

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

Closed
wants to merge 1 commit into from

Conversation

habuma
Copy link
Member

@habuma habuma commented Nov 3, 2024

I noticed that gen_ai.operation.name and gen_ai.system are shown as low-cardinality keys for spring.ai.advisor, but neither of those appear in the metrics. Moreover, I confirmed that are aren't included as low-cardinality keys in either DefaultAdvisorObservationConvention or AdvisorObservationDocumentation. So this PR removes them from the documentation.

@markpollack
Copy link
Member

@jonatan-ivanov can you review this please?

@markpollack markpollack added the documentation Improvements or additions to documentation label Nov 4, 2024
@jonatan-ivanov
Copy link
Member

gen_ai.operation.name is there as an enum:

But it is called AI_OPERATION_TYPE. Type vs. name puzzles me a bit (it seems it is called type everywhere except that value in the enum). It also seems to be used in certain cases, there are even integration tests for it. I would follow through the usage since it seems DefaultChatClientObservationConvention uses it (with other conventions) and if the keyvalue is added metrics should also have this tag:

@Override
public KeyValues getLowCardinalityKeyValues(ChatClientObservationContext context) {
return KeyValues.of(aiOperationType(context), aiProvider(context), springAiKind(), stream(context));
}
protected KeyValue aiOperationType(ChatClientObservationContext context) {
return KeyValue.of(ChatModelObservationDocumentation.LowCardinalityKeyNames.AI_OPERATION_TYPE,
context.getOperationMetadata().operationType());
}

It seems spring.ai.advisor is not a tag on the metric but the name of it:

public static final String DEFAULT_NAME = "spring.ai.advisor";
private final String name;
public DefaultAdvisorObservationConvention() {
this(DEFAULT_NAME);
}

Though metrics are only created when you trigger the operation that records the observation, can it happen that your use-case did not trigger it?

@habuma
Copy link
Member Author

habuma commented Nov 15, 2024

It's been awhile since I created this and thought about it, but..

It's not that I don't see the spring.ai.advisor metric. I do see it when stuff flows through an advisor. It's just that I never see those two keys. For example, I just ran a request through ChatClient in a situation where there are a few advisors in play and here's what I got back from spring.ai.advisor:


  "name": "spring.ai.advisor",
  "baseUnit": "seconds",
  "measurements": [
    {
      "statistic": "COUNT",
      "value": 3.0
    },
    {
      "statistic": "TOTAL_TIME",
      "value": 7.119742541999999
    },
    {
      "statistic": "MAX",
      "value": 4.183466542
    }
  ],
  "availableTags": [
    {
      "tag": "spring.ai.advisor.type",
      "values": [
        "AROUND"
      ]
    },
    {
      "tag": "spring.ai.kind",
      "values": [
        "advisor"
      ]
    },
    {
      "tag": "spring.ai.advisor.name",
      "values": [
        "CallAroundAdvisor",
        "VectorStoreChatMemoryAdvisor",
        "QuestionAnswerAdvisor"
      ]
    },
    {
      "tag": "error",
      "values": [
        "none"
      ]
    }
  ]
}

Neither of the two keys I mentioned are in this response. Since they are documented as low-cardinality keys, I'd expect them to be there. And yes, I see from the code you showed where aiOperationType() should've put the gen_ai.operation.name key in there, so I'm confused why I'm not seeing it. And I see nothing to suggest that the gen_ai.system should've been included.

Also, this most recent attempt was done moments ago using a local build of the very latest snapshot code (to take advantage of the fix for #1738 / #1742 ).

@ThomasVitale
Copy link
Contributor

There's probably some error in the code if those keys are not populated, good catch! The reason for having those low cardinality keys is to enable the integration of Spring AI with observability/evaluation platforms following the OpenTelemetry Semantic Conventions for Generative AI Systems. I'll work soon on a sample app demonstrating integrations with some of those platforms with the hope to clarify the usage and review the current observability setup in Spring AI.

The gen_ai.operation.name is named like that because of the Semantic Conventions, but the name is poorly chosen, so that's why internally in Spring AI we call that an "operation type". We did something similar for gen_ai.system, which we call a "provider" in Spring AI when it's meant to represent a model provider ("system" is not a good name, too generic).

@tzolov tzolov self-assigned this Nov 19, 2024
tzolov added a commit to tzolov/spring-ai that referenced this pull request Nov 19, 2024
…bservations

Add AI_OPERATION_TYPE and AI_PROVIDER as low cardinality key names to advisor observations.
The advisor AI_OPERATION_TYPE is set to 'framework' and the AI_PROVIDER to 'spring_ai'.

Resolves spring-projects#1660
@ilayaperumalg
Copy link
Member

@habuma Closed this PR as the underlying issue is fixed now - both the keys are updated for the Advisor observation.

leijendary pushed a commit to leijendary/spring-ai that referenced this pull request Jan 24, 2025
…bservations

Add AI_OPERATION_TYPE and AI_PROVIDER as low cardinality key names to advisor observations.
The advisor AI_OPERATION_TYPE is set to 'framework' and the AI_PROVIDER to 'spring_ai'.

Resolves spring-projects#1660

Signed-off-by: leijendary <[email protected]>
leijendary pushed a commit to leijendary/spring-ai that referenced this pull request Jan 24, 2025
…bservations

Add AI_OPERATION_TYPE and AI_PROVIDER as low cardinality key names to advisor observations.
The advisor AI_OPERATION_TYPE is set to 'framework' and the AI_PROVIDER to 'spring_ai'.

Resolves spring-projects#1660

Signed-off-by: leijendary <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants