Skip to content
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

added span-link normalization and fixed errors in getTransaction #2

Conversation

bryce-b
Copy link

@bryce-b bryce-b commented Sep 25, 2024

No description provided.

],
},
};
return (fields[SPAN_LINKS_TRACE_ID] as string[]).map((v, index) => {
Copy link
Author

Choose a reason for hiding this comment

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

not sure if this works for OTel

Choose a reason for hiding this comment

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

Thanks for scaffolding this. I don't see a way to test it against real data so far.

Copy link
Owner

@miloszmarcinkowski miloszmarcinkowski left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could you please rebase your branch onto mine once I fetch changes from Jenny's branch?

@@ -369,7 +365,8 @@ export const transactionMapping = (fields: Partial<Record<string, unknown[]>>) =
outcome: normalizeValue<EventOutcome>(fields[EVENT_OUTCOME]),
},
processor: {
event: normalizeValue<string>(fields[PROCESSOR_EVENT]),
event: normalizeValue<'transaction'>(fields[PROCESSOR_EVENT]),
name: normalizeValue<'transaction'>(fields[PROCESSOR_EVENT]),

Choose a reason for hiding this comment

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

Suggested change
name: normalizeValue<'transaction'>(fields[PROCESSOR_EVENT]),
name: normalizeValue<'transaction'>(fields[PROCESOR_NAME]),

],
},
};
return (fields[SPAN_LINKS_TRACE_ID] as string[]).map((v, index) => {

Choose a reason for hiding this comment

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

Thanks for scaffolding this. I don't see a way to test it against real data so far.

@miloszmarcinkowski miloszmarcinkowski force-pushed the 192749-refactor-the-normalize-function-into-per-data-structure-serialization-logic branch from f3c89ca to 20a7b79 Compare September 26, 2024 15:42
@miloszmarcinkowski
Copy link
Owner

I rebased my branch onto Jenny's. I see that it generated more conflicts in this PR. Let me know if you need any help from my side.

@bryce-b bryce-b force-pushed the 192749-refactor-the-normalize-function-into-per-data-structure-serialization-logic branch from 1371bd0 to 02e8065 Compare September 26, 2024 20:36
@@ -573,8 +575,27 @@ export const serviceMetadataIconsMapping = (
name: normalizeValue<string>(fields[CLOUD_PROVIDER]),
},
},
container: normalizeValue<Container>(fields[CONTAINER]),
kubernetes: normalizeValue<Kubernetes>(fields[KUBERNETES]),
container: {
Copy link
Author

@bryce-b bryce-b Sep 26, 2024

Choose a reason for hiding this comment

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

@miloszmarcinkowski
There's an issue that needs to be resolved here. The icons only check if kubernetes: is not null, so this structure will result in the kubernetes Icon always being shown. i.e. if there are no kubernetes related fields this structure will show up as

kubernetes: {
  pod: {
   name: undefined,
   id: undefined,
   }, 
   ....
}

What would the best pattern in javascript be to conditionally add each field? something like:

 kubernetes: {
      pod: {
       normalizeValue<string>(fields[KUBERNETES_POD_NAME]) ? name: normalizeValue<string>(fields[KUBERNETES_POD_NAME]) : null,
...
      },

We may need to reconsider other objects build in this file due to this issue.

Choose a reason for hiding this comment

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

Good point. The trade-off is that we need to select one property that will define existence for entire object. For given:

{
  'kubernetes.pod.id': ['id'],
}

this code

kubernetes: {
  ...(normalizeValue<string>(fields[KUBERNETES_POD_NAME])
    ? {
        pod: {
          name: normalizeValue<string>(fields[KUBERNETES_POD_NAME]),
          uid: normalizeValue<string>(fields[KUBERNETES_POD_UID]),
        },
      }
    : undefined),
},

will produce:

{
  kubernetes: undefined
}

The effect is that we lose kubernetes.pod.id. Maybe instead we should change condition and rely on kubernetes.pod.name?

@jennypavlova what do you think?

Choose a reason for hiding this comment

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

@bryce-b thank you for solving it

@miloszmarcinkowski miloszmarcinkowski force-pushed the 192749-refactor-the-normalize-function-into-per-data-structure-serialization-logic branch from 294ac0e to 8666c00 Compare September 27, 2024 15:34
miloszmarcinkowski pushed a commit that referenced this pull request Sep 27, 2024
…193441)

## Summary
More files to be regenerated with a different shape since the js-yaml
update: elastic#190678
@bryce-b bryce-b force-pushed the 192749-refactor-the-normalize-function-into-per-data-structure-serialization-logic branch from 70fc3b7 to a08ef93 Compare September 27, 2024 17:43
@bryce-b
Copy link
Author

bryce-b commented Sep 27, 2024

@miloszmarcinkowski I went through and compared these changes to edge-lite and verified all the service meta data icons and details are looking good. The one thing that doesn't look right is the kubernetes namespaces value. I'm not able to figure out where that should be fed through to the container_details element. It almost looks like it was removed in some other PR that hasn't made it to edge-lite yet?

Edge-lite

Screenshot 2024-09-27 at 14 14 09

PR

Screenshot 2024-09-27 at 14 14 13

@miloszmarcinkowski
Copy link
Owner

@bryce-b on OTel sync we decided to move forward with merging PRs as this starts to keep us out of sync. I'll merge it and create a separate comment in my PR with things we discussed here. If you had anything pending, please let me know.

…ructure-serialization-logic' into bryce-192749-refactor-the-normalize-function-into-per-data-structure-serialization-logic

# Conflicts:
#	x-pack/plugins/observability_solution/apm/server/utils/es_fields_mappings.ts
@miloszmarcinkowski miloszmarcinkowski merged commit b4c5a95 into miloszmarcinkowski:192749-refactor-the-normalize-function-into-per-data-structure-serialization-logic Sep 30, 2024
@bryce-b bryce-b deleted the 192749-refactor-the-normalize-function-into-per-data-structure-serialization-logic branch September 30, 2024 14: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.

2 participants