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

Updated TSDB documentation with additional details #5706

Merged
merged 12 commits into from
Aug 23, 2023

Conversation

agithomas
Copy link
Contributor

@agithomas agithomas commented Mar 28, 2023

Type of change

  • Enhancement

What does this PR do?

Added useful information for integration developers when working with TSDB.

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.

How to test this PR locally

Not applicable

Related issues

Screenshots

Not applicable

@elasticmachine
Copy link

elasticmachine commented Mar 28, 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-08-23T09:04:08.003+0000

  • Duration: 107 min 9 sec

Test stats 🧪

Test Results
Failed 0
Passed 4920
Skipped 10
Total 4930

🤖 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 Mar 28, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (453/453) 💚
Files 96.65% (779/806) 👎 -3.35
Classes 96.65% (779/806) 👎 -3.35
Methods 91.863% (7530/8197) 👎 -8.137
Lines 88.319% (170879/193480) 👎 -11.681
Conditionals 100.0% (0/0) 💚

@agithomas
Copy link
Contributor Author

/test

Copy link
Collaborator

@lalit-satapathy lalit-satapathy left a comment

Choose a reason for hiding this comment

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

LGTM

docs/developer_tsdb_migration_guidelines.md Outdated Show resolved Hide resolved
@lalit-satapathy
Copy link
Collaborator

@agithomas, Can this PR be merged?

@agithomas
Copy link
Contributor Author

@agithomas, Can this PR be merged?

@lalit-satapathy , @jsoriano can you please approve the PR?

Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
@botelastic
Copy link

botelastic bot commented May 18, 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 May 18, 2023
@agithomas
Copy link
Contributor Author

/test

@botelastic botelastic bot removed the Stalled label Jun 6, 2023
@botelastic
Copy link

botelastic bot commented Jul 6, 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 Jul 6, 2023
@botelastic botelastic bot removed the Stalled label Jul 6, 2023
@botelastic
Copy link

botelastic bot commented Aug 5, 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 Aug 5, 2023
@jsoriano
Copy link
Member

jsoriano commented Aug 7, 2023

/test

@botelastic botelastic bot removed the Stalled label Aug 7, 2023
@jsoriano
Copy link
Member

jsoriano commented Aug 7, 2023

@agithomas @lalit-satapathy should we merge this?

@lalit-satapathy
Copy link
Collaborator

There may be some other changes required, @agithomas do we need to add any others update here to make it up-to-date? Let's keep this doc up-to-date.

@agithomas
Copy link
Contributor Author

@lalit-satapathy , can you do a re-review ? I have updated the guidelines with the most recent guidelines.

Copy link
Collaborator

@lalit-satapathy lalit-satapathy left a comment

Choose a reason for hiding this comment

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

I think we need to re-look into these two sentences:

  • A field having type flattened cannot be selected as a dimension field and that paragraph.

  • An enhancement request for Kibana is created to indicate the write index. Until then, refer to the index.time_series.start_time of indices and compare with the current time to identify the write index.

At the end, Can we also refer to a cloud integration also?

docs/developer_tsdb_migration_guidelines.md Show resolved Hide resolved
@agithomas
Copy link
Contributor Author

At the end, Can we also refer to a cloud integration also?

Added link to AWS Redshift integration at the end.

@agithomas
Copy link
Contributor Author

@lalit-satapathy , are you ok with the documentation so that I can merge?


**Warning:** Choosing an insufficient number of dimension fields may lead to data loss
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you refer the reader to the testing section and link the TSDB migration test kit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@constanca-m , can we take that as a separate update to this document? May be you can add that part?

@constanca-m
Copy link
Contributor

Git doesn't let me comment on lines that were not changed...

Additional comments:

  1. When a package upgrade with the above mentioned change is applied, the changes are made only on the index template. This means, the user need to wait until index.time_series.end_time of the current write index before seeing the change, following a package upgrade.

Are we sure of this? I only upgrade the package and the automatic rollover happens. The only instance that did not happen was when I modified the look_ahead_time field.

  1. Known issues section suggestion: Let's just reference the meta issue to avoid having to maintain this document.

  2. Reference to existing package already migrated section: Can we only add the meta issue as well for this?

  3. Testing section: Please let's just link the TSDB testing kit, this way of testing is not our current way of doing it.

  4. Best practices section:

Use Lens as the preferred visualisation type.

Is this related to TSDB?

Co-authored-by: Constança Manteigas <113898685+constanca-m@users.noreply.github.com>
@agithomas
Copy link
Contributor Author

Git doesn't let me comment on lines that were not changed...

Additional comments:

  1. When a package upgrade with the above mentioned change is applied, the changes are made only on the index template. This means, the user need to wait until index.time_series.end_time of the current write index before seeing the change, following a package upgrade.

Are we sure of this? I only upgrade the package and the automatic rollover happens. The only instance that did not happen was when I modified the look_ahead_time field.

It was the case earlier. I shall re-test this to validate the relevancy of this point.

  1. Known issues section suggestion: Let's just reference the meta issue to avoid having to maintain this document.
  2. Reference to existing package already migrated section: Can we only add the meta issue as well for this?
    Already present. Please refer the section "Other known issues". It will take to the meta issue.
  3. Testing section: Please let's just link the TSDB testing kit, this way of testing is not our current way of doing it.

@constanca-m , you may add this part in a separate PR.

  1. Best practices section:

Use Lens as the preferred visualisation type.

Is this related to TSDB?

Yes, it is. Some of the errors associated with the usage of counter type can only be seen when using Lens visualistion.

@agithomas
Copy link
Contributor Author

4. Testing section: Please let's just link the TSDB testing kit, this way of testing is not our current way of doing it.

I do not think that there exists a fault in comparing the records, especially when the comparison is easy. Record count based check still happens. TSDB testkit can be a supplementary solution.

@agithomas
Copy link
Contributor Author

@constanca-m , i have addressed all the review comments, except those related to test kit. Can the change be merged?

Copy link
Contributor

@constanca-m constanca-m left a comment

Choose a reason for hiding this comment

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

Yes! I will likely open a PR then to include the testing kit, but for now the changes here seem enough.

@agithomas
Copy link
Contributor Author

/test

@agithomas agithomas merged commit d808ae0 into elastic:main Aug 23, 2023
4 checks passed
gizas pushed a commit that referenced this pull request Sep 5, 2023
* Updated TSDB documentation with additional details

* Updated links to the integration examples

* Update docs/developer_tsdb_migration_guidelines.md

Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>

* Updated with recent guidelines

* Added reference to AWS Redshift

* Updated kibana.version guidelines

* Update docs/developer_tsdb_migration_guidelines.md

Co-authored-by: Constança Manteigas <113898685+constanca-m@users.noreply.github.com>

* Updated link to default max limit of TSDB dimensions. Updated Automatic rollover section

* Updated link to default max limit of TSDB dimensions.

* Updated the link within the document

---------

Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
Co-authored-by: Constança Manteigas <113898685+constanca-m@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSDB] Update TSDB guidelines document for Integration developers
5 participants