-
Notifications
You must be signed in to change notification settings - Fork 724
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
Gigamon (ECOINT-9) #2356
base: master
Are you sure you want to change the base?
Gigamon (ECOINT-9) #2356
Conversation
@vishalshah-dd , i have made respective changes as you suggested. Could you please review and let us know ? |
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.
Requesting changes:
- The namespacing of Gigamon on the custom facets was done on the name and not the path.
- The grok expression to extract the date isn't working in the pipeline. It should be on the
ts
attribute. I don't think this was tested in your custom pipeline, can you please make the changes and test before requesting a review for that?
gigamon/assets/logs/gigamon.yaml
Outdated
- type: grok-parser | ||
name: Map 'ts' to 'TimeStamp' | ||
enabled: true | ||
source: message |
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.
This isn't right, it should be ts
, the source attribute where we're applying the grok expression. The message
today has the entire contents. The sample should also be changed to be just the value: Wed Jul 31 12:05:39 2024
.
Once done, you should also add a timestamp remapper with the source attribute as whatever you extract out of the grok parser.
Can you make these changes and test it with your test pipeline? To ensure that the dates you're sending in the logs gets correctly set within Datadog? We drop old logs so please make sure the timestamp you're testing with is within the same day you send it for ingestion.
gigamon/assets/logs/gigamon.yaml
Outdated
- facetType: list | ||
groups: | ||
- gigamon | ||
name: Gigamon APP ID |
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.
May have gotten missed in the back-and-forth, but I mentioned it's the path
that needs namespacing with gigamon.
prefix and not the name. The paths need to be unique across all integrations, and that's why we ask contributors to namespace the custom facets.
This one should look like:
- facetType: list
groups:
- gigamon
name: App ID
path: gigamon.app_id
source: log
type: string
With changing the facet, you'll also need a remapper to map app_id
to gigamon.app_id
:
- type: attribute-remapper
name: Map 'app_id' to 'gigamon.app_id'
enabled: true
sources:
- app_id
sourceType: attribute
target: gigamon.app_id
targetType: attribute
preserveSource: false
overrideOnConflict: false
These remappers would be needed for each custom facet that we add the gigamon.
prefix to
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.
@vishalshah-dd , Thanks for the details. Apologize for delay. We were occupied in customer deliverables. Will work on these and will get back to you. Before that, I do have couple of questions on your two queries:
- Is it mandatory to use timestamp ? can we skip that one?
- Should we manually change the facet values there? As per your suggestion, we should change the path with "gigamon." prefix in the facets. However, in the facet creation, i am unable to edit the path as it is coming as drop down. Am i missing any ? Please help.
- Rempapper, i am able to make the changes. however, first we need to change the facet right. Any help on this ?
![Screenshot 2024-08-22 at 3 56 54 PM](https://private-user-images.githubusercontent.com/140075512/360392937-a9e4c467-00ee-47c2-8f9b-81054f34b73e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxNjg2NjQsIm5iZiI6MTcyNjE2ODM2NCwicGF0aCI6Ii8xNDAwNzU1MTIvMzYwMzkyOTM3LWE5ZTRjNDY3LTAwZWUtNDdjMi04ZjliLTgxMDU0ZjM0YjczZS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTEyJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkxMlQxOTEyNDRaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT00ZGYxN2UyYmQxZjgzNTBlNTQyN2RjNTNmZjliMTMwNTQzZjEwOWE3MjM0NTk5YzQ1N2Y1ZTg2Zjc2OTRmNDZiJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.T27xp6V8bMRIM2N5j5V1by18gQt9_nD3mUwwNUYbzTs)
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.
@vishalshah-dd , should we do the below facet type maunally in the yaml file ?
- facetType: list
groups:- gigamon
name: App ID
path: gigamon.app_id
source: log
type: string
- gigamon
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.
Hey @Mrudula-Oruganti-Gigamon jumping in here
- Timestamp is mandatory. Please treat any requested changes from logs as mandatory moving forward.
- You will need to delete each facet. Next, you will remap all the attributes added as facets under the group Gigamon. (see 3) From there, after the remappers have been added, you will create new facets using the remapped attributes.
- You will first remap each attribute then you will create the facets on the newly remapped attributes (attribute paths). ex remapped attribute: gigamon.app_id then new facet will be created for gigamon.app_id
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.
Thanks @emarsha94 , Will work on it and will update it.
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.
@emarsha94 , could you please review #2356 this thread and confirm ? Thanks.
Hi @Mrudula-Oruganti-Gigamon please use this doc which walks through how to remap ts to be the official timestamp: https://docs.datadoghq.com/developers/integrations/log_pipeline/#log-pipeline-requirements This doc walks through exactly how the standard attributes like timestamp should be remapped |
Hi @vishalshah-dd / @emarsha94 , i have made respective changes. could you please review and let us know if any changes are required ? |
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've changed all existing facets to be under "Gigamon" and the
gigamon.
path prefix. This was only supposed to be done to the facets you originally had under "Gigamon". - I see a bunch of new facets added that weren't there before, some of these should not be under "Gigamon" group and are standard facets (one example:
gigamon.src_port
this should be mapped tonetwork.client.port
. You can see a list of standard facets for logs here. Any facet that you have that has the same behavior as these standard facets should reuse the same paths mentioned in the linked doc, and should not be under the "Gigamon" facet group - Need to remove the timestamp facet, and keep the value as just
ts
and then have a date remapper onts
. This should not be saved asgigamon.ts
since it's a standard attribute in Datadog
gigamon/assets/logs/gigamon.yaml
Outdated
groups: | ||
- gigamon | ||
name: Http Code | ||
path: gigamon.http_code |
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.
Wait sorry, why did you change these to be under the "Gigamon" group and namespace? The only changes to add the gigamon.
prefix to the path were the facets that were under the "Gigamon" group. These standard facets you had before were supposed to be unchanged.
I found this version of the file before you made the changes. You had all the other facets correctly handled under the respective facet groups like Web Access
, and DNS
. The only facets that needed appending a gigamon.
prefix to the path were app_name
, dns_host
, and dns_name
. All the other facets were meant to remain unchanged
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.
Thanks @vishalshah-dd for the clarification. I will do respective changes. I will keep the standard attributes as such and will add the gigamon. prefix for the custom attributes.
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 @Mrudula-Oruganti-Gigamon could you please revert your last commit? Once that is done, please let me know and I will advise on next steps.
As Vishal said, it looks like there were a lot of changes that weren't required or requested that have been added to the yaml files. Please revert and I will re-comment on what needs to be changed.
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.
@emarsha94 , I have made changes as per Vishal's suggestion in the latest commit and have added few more attributes and facets as per his statements.
If i revert back those commit, i may need to perform all the other changes like timestamp, adding new facets again. Do you still want me to revert to the one which i did 3months back?
Is there any issue with the latest build ? Please help.
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 Mrudula, it looks like there were a lot of changes that were not asked and additional facets added that don't belong in the groups they were added to. At this point, I believe the next best step is to revert the most recent commit then follow Vishals comments very carefully: #2356 (review)
-You've changed all existing facets to be under "Gigamon" and the gigamon. path prefix. This was only supposed to be done to the facets you originally had under "Gigamon".
- I see a bunch of new facets added that weren't there before, some of these should not be under "Gigamon" group and are standard facets (one example: gigamon.src_port this should be mapped to network.client.port. You can see a list of standard facets for logs here. Any facet that you have that has the same behavior as these standard facets should reuse the same paths mentioned in the linked doc, and should not be under the "Gigamon" facet group
- Need to remove the timestamp facet, and keep the value as just ts and then have a date remapper on ts. This should not be saved as gigamon.ts since it's a standard attribute in Datadog
Please read the above and review the linked documents and update as the above instructs. Examples were provided in this comment on exactly how the formatting should look.
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.
@emarsha94 , That's true. This is what we have done in our latest commit which we did 4days ago. Do we need to do "submit for review" everytime we commit from github? Attaching the updated yaml file (in .txt format) for reference.
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.
@emarsha94 , the comments that were given by @vishalshah-dd and you have been addressed . Please refer to the comments at the below link :
This should be part of the build:
58b166c
please let me know if there is anything else.
gigamon/assets/logs/gigamon.yaml
Outdated
groups: | ||
- gigamon | ||
name: Time Stamp | ||
path: gigamon.ts |
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.
This one too, we don't need to have a facet, and the timestamp isn't supposed to be under a Gigamon prefix or facet group since it's a Datadog standard attribute
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 @vishalshah-dd , i have made the changes as below. Please correct if my understanding is wrong:
Please let me know if any changes are required. Thanks. |
@vishalshah-dd / @emarsha94 , Could you please review and let us know whether we are good with the latest submission? |
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 as per comments
@emarsha94 / @vishalshah-dd , Could you please let us know the status ? |
Hi @Mrudula-Oruganti-Gigamon I am at a team event until Friday this week. I will review the PR Friday or Monday |
Sure @emarsha94 . Thanks for the update. |
Integration with Gigamon
What does this PR do?
Integration with Gigamon
Motivation
What inspired you to submit this pull request?
Review checklist
no-changelog
label attachedAdditional Notes
Anything else we should know when reviewing?