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

[salesforce] Implement setupaudittrail data stream for Salesforce #4356

Merged

Conversation

yug-rajani
Copy link
Contributor

@yug-rajani yug-rajani commented Sep 30, 2022

  • Enhancement

What does this PR do?

  • Generated the skeleton of Salesforce integration package.
  • Added 1 data stream ( setupaudittrail )
  • Added data collection logic.
  • Added the ingest pipelines.
  • Mapped fields according to the ECS schema and added Fields metadata in the appropriate yml files.
  • Added system test cases.
    salesforce-setupaudittrail-1

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.
  • If I'm introducing a new feature, I have modified the Kibana version constraint in my package's manifest.yml file to point to the latest Elastic stack release (e.g. ^7.13.0).

How to test this PR locally

  • Clone integrations repo.
  • Install elastic-package locally.
  • Start elastic stack using elastic-package.
  • Move to integrations/packages/salesforce directory.
  • Run the following command to run tests.

elastic-package test

@yug-rajani yug-rajani self-assigned this Sep 30, 2022
@yug-rajani yug-rajani added enhancement New feature or request Team:Service-Integrations Label for the Service Integrations team Integration:salesforce Salesforce labels Sep 30, 2022
@yug-rajani yug-rajani linked an issue Sep 30, 2022 that may be closed by this pull request
8 tasks
@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine
Copy link

elasticmachine commented Sep 30, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-01-11T07:20:14.872+0000

  • Duration: 16 min 1 sec

Test stats 🧪

Test Results
Failed 0
Passed 24
Skipped 0
Total 24

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Sep 30, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (4/4) 💚
Files 100.0% (4/4) 💚
Classes 100.0% (4/4) 💚
Methods 100.0% (49/49) 💚
Lines 96.844% (982/1014) 👎 -0.119
Conditionals 100.0% (0/0) 💚

@botelastic
Copy link

botelastic bot commented Dec 3, 2022

Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the Stalled label Dec 3, 2022
@botelastic botelastic bot removed the Stalled label Dec 13, 2022
@kush-elastic kush-elastic marked this pull request as ready for review December 13, 2022 15:05
Copy link
Collaborator

@kush-elastic kush-elastic left a comment

Choose a reason for hiding this comment

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

LGTM

@agithomas
Copy link
Contributor

@SubhrataK , kindly review the dashboard layout

@agithomas
Copy link
Contributor

@kush-elastic , kindly check if the pagination logic should be based on created date instead it must be on LogDate Reference : https://github.com/elastic/beats/pull/34065/files

@agithomas
Copy link
Contributor

Not sure if the description is appropriate for the datastream in Readme.

Represents changes you or other admins made in your organization's Setup area for at least the last 180 days.

Use of word 'you' can be avoided
Only admin can make changes to the setup ? can a user with permissions change? If yes, then use of 'admins' is not right.

Not sure about the significance of 180 days here from the point of view of integration and continuous data fetch.

Kindly revisit this taking the help of @SubhrataK

@kush-elastic
Copy link
Collaborator

@kush-elastic , kindly check if the pagination logic should be based on created date instead it must be on LogDate Reference : https://github.com/elastic/beats/pull/34065/files

@agithomas for setupaudittrail we don't have field called LogDate in our response as it doesn't give us Logfile Id instead we get data directly in the first call itself , also we don't have chain call in setupaudittrail so we will be using created date instead.

@agithomas
Copy link
Contributor

@kush-elastic , kindly check if the pagination logic should be based on created date instead it must be on LogDate Reference : https://github.com/elastic/beats/pull/34065/files

@agithomas for setupaudittrail we don't have field called LogDate in our response as it doesn't give us Logfile Id instead we get data directly in the first call itself , also we don't have chain call in setupaudittrail so we will be using created date instead.

@kush-elastic , kindly check if the pagination logic should be based on created date instead it must be on LogDate Reference : https://github.com/elastic/beats/pull/34065/files

@agithomas for setupaudittrail we don't have field called LogDate in our response as it doesn't give us Logfile Id instead we get data directly in the first call itself , also we don't have chain call in setupaudittrail so we will be using created date instead.

cursor:
last_published_setupaudittrail:
value: '[[.last_event.CreatedDate]]'

Is this then needed, if entire data is received in one go? If the reason is we don't get LogData field, then it is understandable that we can't use it. When create_date field is used, does the same problem that was encountered in login_rest and logout_rest arise?

@kush-elastic
Copy link
Collaborator

kush-elastic commented Jan 3, 2023

cursor:
last_published_setupaudittrail:
value: '[[.last_event.CreatedDate]]'
Is this then needed, if entire data is received in one go? If the reason is we don't get LogData field, then it is understandable that we can't use it. When create_date field is used, does the same problem that was encountered in login_rest and logout_rest arise?

We need cursor as we are using the cursor value to query the next set of data:
"SELECT Action,CreatedByContext,CreatedById,CreatedByIssuer,CreatedDate,DelegateUser,Display,Id,ResponsibleNamespacePrefix,Section FROM SetupAuditTrail WHERE CreatedDate > [[.cursor.last_published_setupaudittrail]] ORDER BY CreatedDate ASC NULLS FIRST

We are setting cursor value to create date here as we don't have LogDate here. Also in case of login_rest and logout_rest we were having chaining+pagination and here we are only having pagination. So created date would work as expected.

@SubhrataK
Copy link

SubhrataK commented Jan 9, 2023

Not sure if the description is appropriate for the datastream in Readme.

Represents changes you or other admins made in your organization's Setup area for at least the last 180 days.

Use of word 'you' can be avoided
Only admin can make changes to the setup ? can a user with permissions change? If yes, then use of 'admins' is not right.

Please replace you with "users" everywhere.

@kush-elastic
Copy link
Collaborator

Not sure if the description is appropriate for the datastream in Readme.

Represents changes you or other admins made in your organization's Setup area for at least the last 180 days.

Use of word 'you' can be avoided Only admin can make changes to the setup ? can a user with permissions change? If yes, then use of 'admins' is not right.

Please replace you with "users" everywhere.

Sure @SubhrataK , Have updated you with "users" everywhere.

As per the salesforce documentation it suggest that there is a possiblity of non admin (or other permission user) as well.

"If a delegate such as an admin or customer support representative makes a setup change on behalf of an end user, the Delegate User column shows the delegate's username. "

So removed the use of admin keyword from the data stream description.

@agithomas
Copy link
Contributor

Not sure if the description is appropriate for the datastream in Readme.

Represents changes you or other admins made in your organization's Setup area for at least the last 180 days.
Use of word 'you' can be avoided Only admin can make changes to the setup ? can a user with permissions change? If yes, then use of 'admins' is not right.
Please replace you with "users" everywhere.

Sure @SubhrataK , Have updated you with "users" everywhere.

As per the salesforce documentation it suggest that there is a possiblity of non admin (or other permission user) as well.

"If a delegate such as an admin or customer support representative makes a setup change on behalf of an end user, the Delegate User column shows the delegate's username. "

So removed the use of admin keyword from the data stream description.

I am not sure about the part for at least the last 180 days in the description. I believe, the REST end point will return records from previous frequency period back in time. If yes, the clause above is conflicting and can be omitted. If salesforce decide to change this timeframe in future, this statement will become incorrect.

@kush-elastic
Copy link
Collaborator

Not sure if the description is appropriate for the datastream in Readme.

Represents changes you or other admins made in your organization's Setup area for at least the last 180 days.
Use of word 'you' can be avoided Only admin can make changes to the setup ? can a user with permissions change? If yes, then use of 'admins' is not right.
Please replace you with "users" everywhere.

Sure @SubhrataK , Have updated you with "users" everywhere.
As per the salesforce documentation it suggest that there is a possiblity of non admin (or other permission user) as well.
"If a delegate such as an admin or customer support representative makes a setup change on behalf of an end user, the Delegate User column shows the delegate's username. "
So removed the use of admin keyword from the data stream description.

I am not sure about the part for at least the last 180 days in the description. I believe, the REST end point will return records from previous frequency period back in time. If yes, the clause above is conflicting and can be omitted. If salesforce decide to change this timeframe in future, this statement will become incorrect.

We have referred salesforce documentation and as per salesforce documentation "Represents changes you or other admins made in your org’s Setup area for at least the last 180 days" they have called out explicitly at least the last 180 days

Let me know if you still insist to remove it from the description we can remove it

@SubhrataK
Copy link

We have referred salesforce documentation and as per salesforce documentation "Represents changes you or other admins made in your org’s Setup area for at least the last 180 days" they have called out explicitly at least the last 180 days

In my opinion, since this is mentioned in their official documentation, we should leave it as it is. In case of a change in the frequency/interval, the changes will likely be available in a new API version, which we will have to certify again at that point.

@agithomas
Copy link
Contributor

agithomas commented Jan 10, 2023

Let me know if you still insist to remove it from the description we can remove it

The question is not about salesforce capability .

The question is - on enabling salesforce integration, will the data as old as 180 days be fetched in the initial run?

@kush-elastic
Copy link
Collaborator

Let me know if you still insist to remove it from the description we can remove it

The question is not about salesforce capability .

The question is - on enabling salesforce integration, will the data as old as 180 days be fetched in the initial run?

Yes, on enabling salesforce integration, the data as old as 180 days will be fetched in the initial run if it is present on the salesforce instance. if not it will only fetch the available data at the instance.

We have tried configuring the fresh policy and have successfully collected data as old as 180 days.

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LTGM

@kush-elastic kush-elastic merged commit 48c7291 into elastic:main Jan 13, 2023
@elasticmachine
Copy link

Package salesforce - 0.4.0 containing this change is available at https://epr.elastic.co/search?package=salesforce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:salesforce Salesforce Team:Service-Integrations Label for the Service Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create package for Salesforce integration
5 participants