Skip to content

Conversation

@apps-elastic-gigamon
Copy link
Contributor

@apps-elastic-gigamon apps-elastic-gigamon commented Jul 25, 2025

Proposed commit message :

This is an enhancement change for the mapping of Gigamon Metadata Attributes to corresponding ECS fields

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.

Related issues

@apps-elastic-gigamon apps-elastic-gigamon requested a review from a team as a code owner July 25, 2025 12:55
@andrewkroh andrewkroh added Integration:gigamon Gigamon (Partner supported) Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations] labels Jul 25, 2025
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@efd6
Copy link
Contributor

efd6 commented Jul 27, 2025

/test

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Please use the PR template. Can you provide a proposed commit message to repair that?

type: keyword
- name: http_rtt
type: keyword
type: float
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not OK. Please revert this.

Copy link
Contributor Author

@apps-elastic-gigamon apps-elastic-gigamon Jul 28, 2025

Choose a reason for hiding this comment

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

For a recent use case from customer, we need to perform math operation for this attribute. So, changed the data type from keyword to float

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing this is a breaking change.

Copy link
Contributor

@kcreddy kcreddy Jul 29, 2025

Choose a reason for hiding this comment

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

@apps-elastic-gigamon, any datatype changes not within the same family, for example: keyword family, is a breaking-change because it leads to mapping conflicts for existing users.

If this is must, you will need to update changelog version as 2.0.0 with type: breaking-change. Example:

- version: "3.0.0"
changes:
- description: |
Auth Key (API Key) is now required for all AbuseCH API requests. Requests without authentication will be denied.
See the official announcement: https://abuse.ch/blog/community-first/
type: breaking-change
link: https://github.com/elastic/integrations/pull/14335

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code changes done as per the comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than mutating existing tests, please add new tests that cover the additions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new JSON records as per the comments

# newer versions go on top
- version: "1.7.0"
changes:
- description: Mapping of Gigamon Attributes to ECS fields
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a sentence; suggest "Improve mapping of Gigamon attributes to ECS fields."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are currently mapping the Gigamon attributes with the appropriate ECS fields, rather than making enhancements to the existing mappings.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current text is not a valid English sentence since it has no verb. Please apply the change or something equivalent. The suggested change adds an appropriate verb since this is an improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the description as per comments

type: long
- name: app_id
type: long
type: keyword
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunate, but cannot be changed without a breaking change. Please revert this.

Copy link
Contributor Author

@apps-elastic-gigamon apps-elastic-gigamon Jul 28, 2025

Choose a reason for hiding this comment

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

We have mapped service.id ECS field to this app_id. Hence the data type has to be changed. Also, in the backend we haven't done any math operations using this app_id

Copy link
Contributor

@efd6 efd6 Jul 29, 2025

Choose a reason for hiding this comment

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

Yes, it's unfortunate that the field was originally defined as a long rather than a keyword as is conventional for an ID field, but changing this from a long to a keyword is a breaking change, so making such a change would require a significant benefit to counter the cost of breaking people.

It does need to be a keyword when it is copied to event.id, but that can either be done as a convert to a string, or just by a copy since numeric values can be mapped as keywords.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the changelog.yml accordingly

Copy link
Contributor

Choose a reason for hiding this comment

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

I still do not believe this change is justified.

@kcreddy kcreddy added enhancement New feature or request maintainer:Partner Partner supported integration labels Jul 28, 2025
Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

CI is failing on error: Error: checking package failed: checking readme files are up-to-date failed: files do not match

To fix this CI error, you will need to run elastic-package build and commit the generated docs/README.md.

@apps-elastic-gigamon apps-elastic-gigamon force-pushed the new branch 3 times, most recently from 77c9706 to e65845c Compare July 28, 2025 11:04
@andrewkroh andrewkroh added the documentation Improvements or additions to documentation. Applied to PRs that modify *.md files. label Jul 28, 2025
@kcreddy
Copy link
Contributor

kcreddy commented Jul 29, 2025

/test

@elastic-vault-github-plugin-prod
Copy link

elastic-vault-github-plugin-prod bot commented Jul 29, 2025

🚀 Benchmarks report

Package gigamon 👍(0) 💚(0) 💔(1)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
ami 4694.84 2890.17 -1804.67 (-38.44%) 💔

To see the full report comment with /test benchmark fullreport

@kcreddy
Copy link
Contributor

kcreddy commented Jul 29, 2025

/test

Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

LGTM, to be merged after @efd6 comments are addressed.

- version: "2.0.0"
changes:
- description: Update mapping of Gigamon attributes to align with ECS fields
type: breaking-change
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not believe the balance of benefits/costs supports a breaking change here.

type: long
- name: app_id
type: long
type: keyword
Copy link
Contributor

Choose a reason for hiding this comment

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

I still do not believe this change is justified.

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Reluctantly

@efd6
Copy link
Contributor

efd6 commented Jul 29, 2025

/test

@elasticmachine
Copy link

💚 Build Succeeded

History

@efd6 efd6 merged commit a04f128 into elastic:main Jul 29, 2025
9 checks passed
@elastic-vault-github-plugin-prod

Package gigamon - 2.0.0 containing this change is available at https://epr.elastic.co/package/gigamon/2.0.0/

@andrewkroh andrewkroh added enhancement New feature or request and removed enhancement New feature or request labels Aug 7, 2025
apps-elastic-gigamon added a commit to apps-elastic-gigamon/integrations that referenced this pull request Sep 5, 2025
…#14692) This is an enhancement change for the mapping of Gigamon Metadata Attributes to corresponding ECS fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change documentation Improvements or additions to documentation. Applied to PRs that modify *.md files. enhancement New feature or request Integration:gigamon Gigamon (Partner supported) maintainer:Partner Partner supported integration Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations]

5 participants