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 logout_stream data stream for Salesforce #4942

Closed

Conversation

kush-elastic
Copy link
Collaborator

  • Enhancement

What does this PR do?

  • Generated the skeleton of Salesforce integration package.
  • Added 1 data stream ( logout_stream )
  • 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.

NOTE: The PR has been tested against 8.7.0-SNAPSHOT for review purpose only, it should be tested once with 8.7.0 before merging this PR, so don't merge this PR unless tested against8.7.0

Related issues

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. ^8.7.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

@kush-elastic kush-elastic added enhancement New feature or request Team:Service-Integrations Label for the Service Integrations team Integration:salesforce Salesforce labels Jan 5, 2023
@kush-elastic kush-elastic self-assigned this Jan 5, 2023
@kush-elastic kush-elastic requested a review from a team as a code owner January 5, 2023 14:10
@elasticmachine
Copy link

elasticmachine commented Jan 5, 2023

💚 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-30T11:47:49.654+0000

  • Duration: 16 min 29 sec

Test stats 🧪

Test Results
Failed 0
Passed 29
Skipped 0
Total 29

🤖 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 Jan 5, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (5/5) 💚
Files 100.0% (5/5) 💚
Classes 100.0% (5/5) 💚
Methods 100.0% (61/61) 💚 9.709
Lines 96.754% (1103/1140) 👍 0.778
Conditionals 100.0% (0/0) 💚

@kush-elastic
Copy link
Collaborator Author

/test

@harnish-elastic
Copy link
Contributor

LGTM! Approved

@agithomas
Copy link
Contributor

Kibana dashboard files and screenshots are missing. Any specific reason?

@kush-elastic
Copy link
Collaborator Author

Kibana dashboard files and screenshots are missing. Any specific reason?

As part of the logout_stream PR we don't have any specific visualization the dashboard which were part of logout_rest are the one which will be used by this data stream.

@agithomas
Copy link
Contributor

agithomas commented Jan 24, 2023

As part of the logout_stream PR we don't have any specific visualization the dashboard which were part of logout_rest are the one which will be used by this data stream.

@SubhrataK , i am thinking of a few visualisation here - the count of events, geo distribution, top IP address. What do you recommend?

Reference : https://github.com/elastic/integrations/pull/4323/files#diff-5a0ee4bfbbc9a6a868c82dc9c6311b6be8f4e344061f1f9343c0d569f3399ef3

May be we can have the geo representation of the event.

@kush-elastic
Copy link
Collaborator Author

As part of the logout_stream PR we don't have any specific visualization the dashboard which were part of logout_rest are the one which will be used by this data stream.

@SubhrataK , i am thinking of a few visualisation here - the count of events, geo distribution, top IP address. What do you recommend?

Reference : https://github.com/elastic/integrations/pull/4323/files#diff-5a0ee4bfbbc9a6a868c82dc9c6311b6be8f4e344061f1f9343c0d569f3399ef3

May be we can have the geo representation of the event.

@agithomas The Dashboard which is part of the logout_rest will have the data flowing from both the data stream logout_rest and logout_stream and the dashboard will be populated using the data from both the data streams.

Let me know if you want to add new visualization as part of the logout_stream data stream to the common Logout Dashboard for the Logout (rest and stream)

@agithomas
Copy link
Contributor

As part of the logout_stream PR we don't have any specific visualization the dashboard which were part of logout_rest are the one which will be used by this data stream.

If a user logs out from salesforce with logout_stream and logout_rest enabled and configured, will two logout events create or just one? In what scenario logout_stream gets the data and logout_rest gets the data?

@kush-elastic
Copy link
Collaborator Author

As part of the logout_stream PR we don't have any specific visualization the dashboard which were part of logout_rest are the one which will be used by this data stream.

If a user logs out from salesforce with logout_stream and logout_rest enabled and configured, will two logout events create or just one? In what scenario logout_stream gets the data and logout_rest gets the data?

I don't see a scenario where user enables both the logout_rest and logout_stream as this are the just two different methods for the data collection.

In all scenario logout_rest will collect the data when available in the LogFile Hourly and the logout_stream gets the data as soon as the user performs the action.

Both the data will be stored in the default individual index of the data stream and that will fall under "logs*" index pattern.

If you suggest we can add the NOTE in the Troubleshooting section which says "If there is any data duplication for login and logout( stream and rest), Please disable any one of the data collection mechanism"

description: Collecting logs from Salesforce instances.
title: Collect Salesforce logs using REST API
description: Collecting logs from Salesforce instances using REST API.
- type: cometd
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that cometd based integration offers more datastreams, move this as the first item in the list.

Add a notes / description under httpjson to inform the user to disable the cometd login / logout datastream in case they are enabling httpjson based integration .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As of now we currently have 2 data-streams for cometd input and 4 data-streams for httpjson input so we can keep the httpjson first and then the cometd input.

I was looking for the option to add the NOTE in the integration page but i could not find the appropriate/intuitive place to add this. Let me know your thoughts over the same

However, We can still add NOTE in the README saying that "User need to disable the httpjson(login and logout) data streams if user want to use cometd(login and logout) data streams to avoid data duplication from using both the inputs"

@agithomas
Copy link
Contributor

If you suggest we can add the NOTE in the Troubleshooting section which says "If there is any data duplication for login and logout( stream and rest), Please disable any one of the data collection mechanism"

Please add this information under https://github.com/elastic/integrations/tree/main/packages/salesforce#configuration - to enable either of login_rest / login_stream and either of logout_stream / logout_rest.

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

@botelastic
Copy link

botelastic bot commented Mar 4, 2023

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 Mar 4, 2023
@botelastic
Copy link

botelastic bot commented Apr 4, 2023

Hi! This PR has been stale for a while and we're going to close it as part of our cleanup procedure. We appreciate your contribution and would like to apologize if we have not been able to review it, due to the current heavy load of the team. Feel free to re-open this PR if you think it should stay open and is worth rebasing. Thank you for your contribution!

@botelastic botelastic bot closed this Apr 4, 2023
@kush-elastic kush-elastic reopened this Apr 12, 2023
@botelastic botelastic bot removed the Stalled label Apr 12, 2023
@kush-elastic kush-elastic force-pushed the package_salesforce_logout_stream branch from 8ffd2ef to 1bb4eae Compare April 12, 2023 12:00
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.

5 participants