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.

Sign up for GitHub

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

Jump to bottom

Add tree-based test suite that explores many possible sequences of operations #2

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Add tree-based test suite that explores many possible sequences of operations #2

merged 1 commit into from
Jul 28, 2023

Conversation

djaglowski
Copy link

No description provided.

@djaglowski djaglowski mentioned this pull request Jul 28, 2023
Closed
@VihasMakwana VihasMakwana merged commit 4c918c7 into VihasMakwana:add-trie-fingerprints Jul 28, 2023
2 checks passed
VihasMakwana
VihasMakwana reviewed Jul 28, 2023
View reviewed changes
pkg/stanza/fileconsumer/internal/trie/trie_test.go
ops: []testOp{
del("ABCDEF", true, "trying to delete ABC should not affect ABCDEF"),
has("ABC", true, "should not have been deleted"),
},
Copy link
Owner

Choose a reason for hiding this comment

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

This will return false, as we previously pushed down the value at #219

VihasMakwana
VihasMakwana reviewed Jul 28, 2023
View reviewed changes
pkg/stanza/fileconsumer/internal/trie/trie_test.go
"DeleteAs:ABC": opTree{ // Also remove it as ABC
ops: []testOp{
del("ABC", true, "just confirmed it exists"), // TODO this fails to delete
},
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. Trie will be basically empty, I just confirmed it.

VihasMakwana
VihasMakwana reviewed Jul 28, 2023
View reviewed changes
pkg/stanza/fileconsumer/internal/trie/trie_test.go
ops: []testOp{
has("ABCDEFxyz", true, "recognize ABCDEF w/ xyz appended"),
put("ABCDEFxyz"), // TODO how do we know we need to call this?
has("ABCDEF", false, "ABCDEF should have been pushed down to ABCDEFxyz"),
Copy link
Owner

Choose a reason for hiding this comment

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

It's true that ABCDEF was pushed down, but we still have ABC in trie.
If we match ABCDEF, it will recognize ABC \w DEF appended

VihasMakwana
VihasMakwana reviewed Jul 28, 2023
View reviewed changes
pkg/stanza/fileconsumer/internal/trie/trie_test.go
func del(key string, expect bool, why string) testOp {
return func(t *testing.T, trie *Trie) {
assert.Equalf(t, trie.Delete([]byte(key)), expect, why)
assert.Falsef(t, trie.HasKey([]byte(key)), "called Delete(%s) but HasKey(%s) is still true", key, key)
Copy link
Owner

Choose a reason for hiding this comment

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

Well, this won't necessarily be false.
Consider, A-B-C-D(true)-E-F(true)
Deleting ABCDEF will cause A-B-C-D(true).
Now, if we do HasKey(ABCDEF), it will return true coz ABCD \w EF appended

Copy link
Owner

Choose a reason for hiding this comment

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

Need now worry about losing the data, even if the file gets deleted, we have the reader stored.
Will close out readers after emitting which haven't been redectected in last 3 poll cycles.

VihasMakwana
VihasMakwana reviewed Jul 30, 2023
View reviewed changes
pkg/stanza/fileconsumer/internal/trie/trie_test.go
},
// TODO When we started reading this file, we identified it as ABC.
// However, we have an updated understanding that it is ABCDEF.
// Should we only have to delete ABC, or only ABCDEF, or both?
Copy link
Owner

Choose a reason for hiding this comment

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

We should only allow deleting ABCDEF, as ABC doesn't exist anymore.

@djaglowski djaglowski mentioned this pull request Aug 7, 2023
Merged
VihasMakwana pushed a commit that referenced this pull request Sep 23, 2024
@daidokoro @kentquirk @TylerHelmuth
[processor/transform] Add Conversion Function to OTTL for Exponential…
74b1048 
… Histo --> Histogram (open-telemetry#33824)

## Description

This PR adds a custom metric function to the transformprocessor to
convert exponential histograms to explicit histograms.

Link to tracking issue: Resolves open-telemetry#33827

**Function Name**
```
convert_exponential_histogram_to_explicit_histogram
```

**Arguments:**

- `distribution` (_upper, midpoint, uniform, random_)
- `ExplicitBoundaries: []float64`

**Usage example:**

```yaml
processors:
  transform:
    error_mode: propagate
    metric_statements:
    - context: metric
      statements:
        - convert_exponential_histogram_to_explicit_histogram("random", [10.0, 20.0, 30.0, 40.0, 50.0, 60.0, 70.0, 80.0, 90.0, 100.0]) 
```

**Converts:**

```
Resource SchemaURL: 
ScopeMetrics #0
ScopeMetrics SchemaURL: 
InstrumentationScope  
Metric #0
Descriptor:
     -> Name: response_time
     -> Description: 
     -> Unit: 
     -> DataType: ExponentialHistogram
     -> AggregationTemporality: Delta
ExponentialHistogramDataPoints #0
Data point attributes:
     -> metric_type: Str(timing)
StartTimestamp: 1970-01-01 00:00:00 +0000 UTC
Timestamp: 2024-07-31 09:35:25.212037 +0000 UTC
Count: 44
Sum: 999.000000
Min: 40.000000
Max: 245.000000
Bucket (32.000000, 64.000000], Count: 10
Bucket (64.000000, 128.000000], Count: 22
Bucket (128.000000, 256.000000], Count: 12
        {"kind": "exporter", "data_type": "metrics", "name": "debug"}
```

**To:**

```
Resource SchemaURL: 
ScopeMetrics #0
ScopeMetrics SchemaURL: 
InstrumentationScope  
Metric #0
Descriptor:
     -> Name: response_time
     -> Description: 
     -> Unit: 
     -> DataType: Histogram
     -> AggregationTemporality: Delta
HistogramDataPoints #0
Data point attributes:
     -> metric_type: Str(timing)
StartTimestamp: 1970-01-01 00:00:00 +0000 UTC
Timestamp: 2024-07-30 21:37:07.830902 +0000 UTC
Count: 44
Sum: 999.000000
Min: 40.000000
Max: 245.000000
ExplicitBounds #0: 10.000000
ExplicitBounds #1: 20.000000
ExplicitBounds #2: 30.000000
ExplicitBounds #3: 40.000000
ExplicitBounds #4: 50.000000
ExplicitBounds open-telemetry#5: 60.000000
ExplicitBounds open-telemetry#6: 70.000000
ExplicitBounds open-telemetry#7: 80.000000
ExplicitBounds open-telemetry#8: 90.000000
ExplicitBounds open-telemetry#9: 100.000000
Buckets #0, Count: 0
Buckets #1, Count: 0
Buckets #2, Count: 0
Buckets #3, Count: 2
Buckets #4, Count: 5
Buckets open-telemetry#5, Count: 0
Buckets open-telemetry#6, Count: 3
Buckets open-telemetry#7, Count: 7
Buckets open-telemetry#8, Count: 2
Buckets open-telemetry#9, Count: 4
Buckets open-telemetry#10, Count: 21
        {"kind": "exporter", "data_type": "metrics", "name": "debug"}
```

### Testing

- Several unit tests have been created. We have also tested by ingesting
and converting exponential histograms from the `statsdreceiver` as well
as directly via the `otlpreceiver` over grpc over several hours with a
large amount of data.

- We have clients that have been running this solution in production for
a number of weeks.

### Readme description:

### convert_exponential_hist_to_explicit_hist

`convert_exponential_hist_to_explicit_hist([ExplicitBounds])`

the `convert_exponential_hist_to_explicit_hist` function converts an
ExponentialHistogram to an Explicit (_normal_) Histogram.

`ExplicitBounds` is represents the list of bucket boundaries for the new
histogram. This argument is __required__ and __cannot be empty__.

__WARNING:__

The process of converting an ExponentialHistogram to an Explicit
Histogram is not perfect and may result in a loss of precision. It is
important to define an appropriate set of bucket boundaries to minimize
this loss. For example, selecting Boundaries that are too high or too
low may result histogram buckets that are too wide or too narrow,
respectively.

---------

Co-authored-by: Kent Quirk <[email protected]>
Co-authored-by: Tyler Helmuth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VihasMakwana VihasMakwana VihasMakwana left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@djaglowski @VihasMakwana