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

Document metrics in Tarantool 3.0 #4348

Merged
merged 11 commits into from
Jul 19, 2024
Merged

Document metrics in Tarantool 3.0 #4348

merged 11 commits into from
Jul 19, 2024

Conversation

andreyaksenov
Copy link
Contributor

@andreyaksenov andreyaksenov commented Jul 5, 2024

New runnable samples used to demonstrate metrics API

The following samples are added to the code_snippets folder of the doc repo:

Docs for metrics

Docs are copied from the metrics module repo and updated.

Related docs

Removed roles_cfg from the sharded_cluster_crud sample and updated corresponding docs:

Related tasks

Add redirects for removed/moved topics:

Misc

Redirects:

TarantoolBot:

@andreyaksenov andreyaksenov linked an issue Jul 5, 2024 that may be closed by this pull request
@andreyaksenov andreyaksenov force-pushed the metrics branch 11 times, most recently from 7c108eb to ab5731b Compare July 10, 2024 07:23
@andreyaksenov andreyaksenov linked an issue Jul 10, 2024 that may be closed by this pull request
@andreyaksenov andreyaksenov force-pushed the metrics branch 17 times, most recently from ab7dda1 to a452060 Compare July 11, 2024 12:51
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

So how it will work for Cartridge and Tarantool 2.11?

doc/book/admin/monitoring/images/role-enable.png Outdated Show resolved Hide resolved
Tarantool configuration
-----------------------

**Since:** :doc:`3.0.0 </release/3.0.0>`.
Copy link
Member

Choose a reason for hiding this comment

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

Well, if we treat metrics as a built-in package, these two are expected to be "since 3.1.1" (tarantool/tarantool#10220)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a doc issue for this: #4365

Copy link
Member

Choose a reason for hiding this comment

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

And what's about this version of doc?

doc/reference/configuration/configuration_reference.rst Outdated Show resolved Hide resolved
doc/reference/configuration/configuration_reference.rst Outdated Show resolved Hide resolved
doc/reference/configuration/configuration_reference.rst Outdated Show resolved Hide resolved
doc/reference/configuration/configuration_reference.rst Outdated Show resolved Hide resolved
doc/reference/reference_lua/metrics.rst Outdated Show resolved Hide resolved
doc/reference/reference_lua/metrics.rst Show resolved Hide resolved
@andreyaksenov
Copy link
Contributor Author

So how it will work for Cartridge and Tarantool 2.11?

Given that Cartridge is deprecated in v3.0, we remove all content related to Cartridge from docs.
2.11 users still can read docs for metrics and Cartridge here:

https://www.tarantool.io/en/doc/2.11/book/monitoring/

Choose a reason for hiding this comment

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

We're removing all cartridge-related content, but we still have cartridge pictures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the catch, will drop

@andreyaksenov andreyaksenov linked an issue Jul 16, 2024 that may be closed by this pull request
Copy link
Contributor

@p7nov p7nov left a comment

Choose a reason for hiding this comment

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

The Administration > Monitoring section LGTM.
Some minor comments and questions.

Example on GitHub: `sharded_cluster_crud_metrics <https://github.com/tarantool/doc/tree/latest/doc/code_snippets/snippets/sharding/instances.enabled/sharded_cluster_crud_metrics>`_

Tarantool allows you to configure and expose its :ref:`metrics <metrics-reference>` using a :ref:`YAML configuration <configuration>`.
You can also use the :ref:`metrics <metrics-api_reference>` module to create and collect custom metrics.
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
You can also use the :ref:`metrics <metrics-api_reference>` module to create and collect custom metrics.
You can also use the built-in :ref:`metrics <metrics-api_reference>` module to create and collect custom metrics.

This addition may be helpful for users of previous versions where metrics wasn't on board.

-------------------

To configure metrics, use the :ref:`metrics <configuration_reference_metrics>` section in a cluster configuration.
The configuration below enables all metrics, excluding :ref:`Vinyl <metrics-reference-vinyl>`-specific ones:
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 configuration below enables all metrics, excluding :ref:`Vinyl <metrics-reference-vinyl>`-specific ones:
The configuration below enables all metrics excluding :ref:`vinyl <metrics-reference-vinyl>`-specific ones:

vynil seems to be all-lowercase: https://docs.d.tarantool.io/en/doc/metrics/concepts/engines/vinyl/

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, excluding.. is a restrictive clause, so no comma is needed.

:language: yaml
:dedent:

The ``metrics.labels`` option accepts the predefined :ref:`{{ instance_name }} <configuration_predefined_variables>` variable to use an instance name as a :ref:`label <metrics-api_reference-labels>` to be added to every observation.
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 ``metrics.labels`` option accepts the predefined :ref:`{{ instance_name }} <configuration_predefined_variables>` variable to use an instance name as a :ref:`label <metrics-api_reference-labels>` to be added to every observation.
The ``metrics.labels`` option accepts the predefined :ref:`{{ instance_name }} <configuration_predefined_variables>` variable. This adds an instance name as a :ref:`label <metrics-api_reference-labels>` to every observation.

Just an idea: shorter sentences, simpler wording.

expirationd:
cfg:
metrics: true

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we provide some "learn more" for this section? For example, read the module docs or ..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope, the link to expirationd readme should be enough. Moreover, there is no additional info in the expirationd readme about metrics.

Exposing metrics
----------------

To expose metrics in different formats, you can use a third-party `metrics-export-role <https://github.com/tarantool/metrics-export-role>`__ role.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if third-party is correct here. It look like an official Tarantool component, judging by the GitHub repo. The role may not be available out of the box, but it's from the Tarantool party.

Collect metrics with server agents
-------------------------------------------------------------------------------

To collect metrics for Prometheus, first set up metrics output with ``prometheus`` format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like third-level headings would be useful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree

Comment on lines 116 to 117
Be sure to include each label key as ``label_pairs_<key>`` so it will be
extracted with the plugin.
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
Be sure to include each label key as ``label_pairs_<key>`` so it will be
extracted with the plugin.
Be sure to include each label key as ``label_pairs_<key>`` to extract it
with the plugin.

.. image:: images/grafana_import_setup.png
:align: left

You can choose datasource and datasource variables after import.
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
You can choose datasource and datasource variables after import.
You can choose the data source and data source variables after import.

https://grafana.com/docs/grafana/latest/datasources/

@@ -0,0 +1,165 @@
.. _monitoring-grafana_dashboard-page:

===============================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to convert heading markup to the format we use across the docs.

Lua memory
"""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""

The Lua memory is limited to 2 GB per instance. Monitoring ``tnt_info_memory_lua``
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still the case? AFAIR this was a limitation before GC64, please recheck with devs.

@xuniq xuniq self-requested a review July 18, 2024 11:48
Copy link
Contributor

@xuniq xuniq left a comment

Choose a reason for hiding this comment

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

Reference sections, API, README -> LGTM, left some minor comments

:header-rows: 0

* - ``tnt_election_state``
- election state (mode) of the node.
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
- election state (mode) of the node.
- Election state (mode) of the node.

Comment on lines +761 to +762
- the transactions themselves (TXN section)
- MVCC
Copy link
Contributor

Choose a reason for hiding this comment

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

The list formatting is broken
Снимок экрана 2024-07-19 в 10 58 37

Comment on lines 781 to 787
* ``total``
The number of bytes that are allocated for the statements of all current transactions.
* ``average``
Average bytes used by transactions for statements
(`txn.statements.total` bytes / number of open transactions).
* ``max``
The maximum number of bytes used by one the current transaction for statements.
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
* ``total``
The number of bytes that are allocated for the statements of all current transactions.
* ``average``
Average bytes used by transactions for statements
(`txn.statements.total` bytes / number of open transactions).
* ``max``
The maximum number of bytes used by one the current transaction for statements.
* ``total`` -- the number of bytes that are allocated for the statements of all current transactions.
* ``average`` -- average bytes used by transactions for statements
(`txn.statements.total` bytes / number of open transactions).
* ``max`` -- the maximum number of bytes used by one the current transaction for statements.

I suggest one of the following formatting:

  • total -- the number of bytes...
  • total: the number of bytes ...

Also, please check this list formatting in other reference - let's make it consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated using the second approach (with :).

Comment on lines +863 to +864
- ``retained`` tuples - they are no longer in the index, but MVCC does not allow them to be removed.
- ``stories`` - MVCC is based on the story mechanism, almost every tuple has a story.
Copy link
Contributor

Choose a reason for hiding this comment

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

The list formatting is broken here

@andreyaksenov andreyaksenov merged commit 692508b into latest Jul 19, 2024
1 check passed
@andreyaksenov andreyaksenov deleted the metrics branch July 19, 2024 09:38
@andreyaksenov andreyaksenov mentioned this pull request Jul 25, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants