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

Fix static decomposition #1456

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

Conversation

narategithub
Copy link
Collaborator

@narategithub narategithub commented Sep 27, 2024

ldms-test/ldmsd_decomp_test was failing and the following bugs were discovered:

  • "type" from the configuration was ignored,
    • "fill" was not working because "type" was ignored,
  • the ARRAY values were ignored.

This pull request addressed the discovered issued as well as keeping the destination rows consistent within ldmsd process life span as follows:

  • The type of a destination column is stored in cfg_col->type.
    • If "type" of a column is specified in the config file, cfg_col->type is set
      to the specified "type".
    • Otherwise, cfg_col->type is discovered from the metric in the set being
      processed. If it cannot be resolved, the row is skipped.
  • If the type of the metric from src (e.g. from set) is not the same as
    the same type of the dst, the value will be coerced. The array values are
    coerced element-by-element.

(old texts)
In addition, four ldms_mval interfaces were also introduced:

  • ldms_mval_set_int()
  • ldms_mval_set_real()
  • ldms_mval_array_set_int()
  • ldms_mval_array_set_real()
    The APIs received the biggest size of integer or real number. The conversion of values are handled by the compiler.

@tom95858
Copy link
Collaborator

I think we need to rethink this. We don't need the type because it can be inferred from the destination mval. That's why the decomp parsing logic was moved until after we had the 1st set handle.

@tom95858
Copy link
Collaborator

@narategithub Can you point me at the test failures and I will take a look?

@tom95858
Copy link
Collaborator

@narategithub sorry nevermind the test case is in the pull request

@tom95858
Copy link
Collaborator

@narategithub can you remind me how to set up the containers

@narategithub
Copy link
Collaborator Author

@tom95858 FYI, this change won't require "type". If the config does not specify "type", it will be inferred. However, if "type" is specified in the config, the top-of-tree code ignored it. This patch just changed it to take the "type" if it is in the config.

I'll email you when I finished setting up the test on cygnus (it is building ...).

@narategithub narategithub marked this pull request as draft September 30, 2024 15:44
@narategithub
Copy link
Collaborator Author

The array value assignment was posted in #1457 . I convert this PR to Draft to keep it open for discussion.

This patch allows static decomposition configuration to easily access
the record member as follows:

```
	...
        { "src":"netdev_list[rx_bytes]" },
	...
```
@narategithub narategithub marked this pull request as ready for review October 30, 2024 14:35
@narategithub
Copy link
Collaborator Author

@tom95858 This is ready for your review.

Narate Taerat added 3 commits November 5, 2024 10:07
If "fill" was correctly specified, `cfg_col->fill` will have a value.
Otherwise, `cfg-col->fill` will be `NULL`. We should use `cfg_col->fill`
to determine if "fill" was present.
The "fill" string value was handled incorrectly. This patch fixes the
bug.
Test sampler access raw metric value with `mval->v_u64` and increment
that value by 1. This works for integer values. When `mval` contains
float or double, `mval->v_u64` became big number due to IEEE 754
binary32 or binary64 format. As such, `test_sampler` should use
`ldms_mval_as_u64()` to coerce the `mval` to u64 properly.
@narategithub
Copy link
Collaborator Author

This PR has been updated to includes "fill" patches #1500 and #1499 (since it depends on a new interface ldms_mval_as_u64()). I've tested this with the following test cases from ldms-test (branch master):

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.

2 participants