-
Notifications
You must be signed in to change notification settings - Fork 450
[RFC 0052] Stage 2: Update additional GenAI fields #2532
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
base: main
Are you sure you want to change the base?
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Documentation changes preview: https://docs-v3-preview.elastic.dev/elastic/ecs/pull/2532/reference/ |
🔍 Preview links for changed docs |
8ece5e6
to
9b177e4
Compare
| [ECS](/reference/ecs-ecs.md) | Meta-information specific to ECS. | | ||
| [ELF Header](/reference/ecs-elf.md) | These fields contain Linux Executable Linkable Format (ELF) metadata. | | ||
| [Email](/reference/ecs-email.md) | Describes an email transaction. | | ||
| [Entity](/reference/ecs-entity.md) | Fields to describe various types of entities across IT environments. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rebased off main and then generated the files, not sure why these changes are still showing up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was bug unrelated to this in how the entity fields are generated. The fix for that isn't merged yet.
Don't worry about these changes in your PR for now, it'll go away once the other fix is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this done and I can regenerate the documents? cc @trisch-me
1b04b05
to
22cdf78
Compare
rfcs/text/0052/gen_ai.yaml
Outdated
description: The system message or instructions provided to the GenAI model separately from the chat history. | ||
example: TODO | ||
level: extended | ||
beta: This field reuse is beta and subject to change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why it’s field reuse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the wording from @mjwolf for the initial batch of fields: #2475 (comment)
However let me know if this needs update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was incorrect before, the text should be This field is beta and subject to change.
for all the beta fields here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed all occurrences, thanks for catching that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as stage 2 is a final stage, please update all examples and generate all fields
The main difference to keep in mind is that flattened does not retain an association between the properties of an array of objects, while nested does. Taking an example from the otel repo:
With flattened, it would not be possible to query for something like
The association between the So whether it should be flattened or nested really depends on what we want to do with this data later on. I'm not an expert on the genai stuff, not sure what to do about it... |
2b0a2e2
to
8a89add
Compare
@flash1293 thanks a lot for explaining the tradeoffs. For the fields where it would be more useful to keep the associations and have more cases for searching, I changed the field type to |
gen_ai.tool.definitions | (Looking for feedback) nested | (Part of invoke_agent span) The list of source system tool definitions available to the GenAI agent or model. | ||
gen_ai.tool.call.arguments | (Looking for feedback) nested | (Part of OTel execute_tool span) Parameters passed to the tool call. | ||
gen_ai.tool.call.result | (Looking for feedback) nested | (Part of OTel execute_tool span) The result returned by the tool call (if any and if execution was successful). | ||
gen_ai.tool.call.result | (Looking for feedback) flattened | (Part of OTel execute_tool span) The result returned by the tool call (if any and if execution was successful). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can take out (Looking for feedback)
if the types have been decided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, apologies, I didn't realize this file had to be updated as well. Let me go and refresh all these files so that they are all up to date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Thanks @susan-shu-c - side note, do we have cases of existing nested and flattened ECS fields already? |
Hi @flash1293 there are a few: Flattened: Nested: |
Thanks for the comments all. I've updated the following:
However, when I try to run Also getting a failure in tests
OTel reference: https://opentelemetry.io/docs/specs/semconv/registry/attributes/gen-ai/#gen-ai-output-messages |
1. What does this PR do?
2. Which ECS fields are affected/introduced?
Changes based on OTel:
3. Why is this change necessary?
4. Have you added/updated documentation?
YES / NO / N/A
5. Have you built ECS and committed any newly generated files?
YES / NO
6. Have you run the ECS validation tests locally?
YES / NO
7. Anything else for the reviewers?
Looking for feedback
[Edit: see comment]
For the fields where it would be more useful to keep the associations and have more cases for searching, I changed the field type to nested, and for those that don't need the associations and probably don't need nested searching, I changed them to flattened.
For most of the fields, they are lists of .json objects, or .json objects. For fields whose content could be very long (input.messages
,output.messages
), I have proposed that they are theflattened
type due to costs.via docs for
nested
type:Though as I am not a subject matter expert on the field types and efficiency, looking for additional feedback or comments.
Commit Message