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

[microsoft-exchange-online-message-trace] - Added support for sliding window attributes and updated default interval values. #12239

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ShourieG
Copy link
Contributor

@ShourieG ShourieG commented Jan 7, 2025

Type of change

  • Enhancement

Proposed commit message

Some scenarios demand for a configurable time window with an initial interval and an end interval. To allow this, a configurable time window has been introduced along with updates to necessary default interval values.

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.
  • I have verified that Kibana version constraints are current according to guidelines.
  • I have verified that any added dashboard complies with Kibana's Dashboard good practices

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

Screenshot 2025-01-10 at 6 53 22 PM

Under Advanced Options

Screenshot 2025-01-10 at 6 53 54 PM

@ShourieG ShourieG self-assigned this Jan 7, 2025
@ShourieG ShourieG added integration Label used for meta issues tracking each integration Integration:microsoft_exchange_online_message_trac Microsoft Exchange Online Message Trace enhancement New feature or request Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations] labels Jan 7, 2025
@elastic-vault-github-plugin-prod

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@ShourieG ShourieG requested a review from chrisberkhout January 7, 2025 15:28
Copy link
Contributor

@chrisberkhout chrisberkhout left a comment

Choose a reason for hiding this comment

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

The logic of this looks good.

In a separate PR we should also remove the look-back logic and have it save the time of the last data seen rather than the time of the last execution.

As a name, "End Interval" is a bit confusing for someone who isn't looking at how the request is built. What do you think about this?

- name: min_age
  type: text
  title: Minimum Age
  description: |
    Logs will not be requested until they are at least this old.
    This value should...
    Supported units for this parameter are h/m/s.

@@ -45,9 +45,13 @@ In order to continue using the Microsoft Exchange Online Message Trace you will
- For configuring `Local Domains` you can check your [Microsoft Admin Exchange Center](https://admin.exchange.microsoft.com/) for the domains
available in your organization. They are usually under the sections [Accepted Domains](https://admin.exchange.microsoft.com/#/accepteddomains) and [Remote Domains](https://admin.exchange.microsoft.com/#/remotedomains).

- The default `Polling Interval` and `Initial Interval` values are configured to `1h`, you can however change these to your required values. The look-back
- The default `Polling Interval` is configured to `1h` and `Initial Interval` to `48h`, you can however change these to your required values. The look-back
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- The default `Polling Interval` is configured to `1h` and `Initial Interval` to `48h`, you can however change these to your required values. The look-back
- The default `Interval` for polling is configured to `1h` and `Initial Interval` to `48h`, you can however change these to your required values. The look-back

The title used for the interval variable is just "Interval"

description: |
The time window starting from the initial interval up to when the logs would be pulled. This value should be always lesser than the interval.
Supported units for this parameter are h/m/s.
default: 24h
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the default should be 1h, since I think I saw in the documentation or something that the status is usually available in an hour. If a user wants 24h to match what they've done elsewhere they can override the default.

type: text
title: End Interval
description: |
The time window starting from the initial interval up to when the logs would be pulled. This value should be always lesser than the interval.
Copy link
Contributor

Choose a reason for hiding this comment

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

This value should be always lesser than the interval.

Also true of initial interval, to avoid request ranges where the end is before the start.

If we remove the lookback logic (in a different PR), this can be cleaned up. Then it will only be initial interval. And then we can probably just use initial interval + end interval to avoid the complication entirely.

value of `Initial Interval` should not exceed `200 hours` as this might cause unexpected errors with the API.

- The default `End Interval` is configured to `24h`, you can however change these to your required values. The `End Interval` was introduced to allow a sliding
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a default of 0s to keep existing functionality the same for current users? Else this would be a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ideally that should be the case, still having some discussions on this internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been reduced to 1h as a default in order to avoid logs with the "gettingStatus" state which a lot of users were complaining about.

@ShourieG ShourieG marked this pull request as ready for review January 10, 2025 13:20
@ShourieG ShourieG requested a review from a team as a code owner January 10, 2025 13:20
@elasticmachine
Copy link

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

@ShourieG
Copy link
Contributor Author

@chrisberkhout, I've added the suggested changes.

@ShourieG
Copy link
Contributor Author

/test

1 similar comment
@ShourieG
Copy link
Contributor Author

/test

@ShourieG
Copy link
Contributor Author

@chrisberkhout, could you review the updated PR once

@elasticmachine
Copy link

elasticmachine commented Jan 13, 2025

💔 Build Failed

Failed CI Steps

History

cc @ShourieG

Copy link
Contributor

@chrisberkhout chrisberkhout 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.

My preference would be for a boolean setting to drop "Getting Status", but this way will work too.

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:microsoft_exchange_online_message_trac Microsoft Exchange Online Message Trace integration Label used for meta issues tracking each integration Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants