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

Eliminate multi-data point timeseries #77

Conversation

tigrannajaryan
Copy link
Member

Resolves #54

After a discussion we came to conclusion that there is no good use case
for having multiple data points. All possible ways to combine data points
that we imagined are either very expensive or make no semantic sense.

This commit eliminates timeseries that can have multiple data points and replaces
them with single data points.

If in the future we discover a use case for multiple data points we have 2
possible paths to extend the proto:

  • Add a corresponding points repeated field in the Int64DataPoint.
  • Introduce a different message type that is capable of storing multiple data
    points and assign it a different type in the MetricDescriptor.Type enum.

There are probably other ways as well.

Resolves open-telemetry#54

After a discussion we came to conclusion that there is no good use case
for having multiple data points. All possible ways to combine data points
that we imagined are either very expensive or make no semantic sense.

This commit eliminates timeseries that can have multiple data points and replaces
them with single data points.

If in the future we discover a use case for multiple data points we have 2
possible paths to extend the proto:

- Add a corresponding points repeated field in the Int64DataPoint.
- Introduce a different message type that is capable of storing multiple data
  points and assign it a different type in the MetricDescriptor.Type enum.

There are probably other ways as well.
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Can you split the removing of the generated code in a separate PR?

@tigrannajaryan
Copy link
Member Author

Can you split the removing of the generated code in a separate PR?

@bogdandrutu I do not understand, can you clarify? I did not remove any code, just re-generated it to match proto changes.

@bogdandrutu
Copy link
Member

@tigrannajaryan I think I understand what happens. You actually regenerated the files. This is so hard to review. I think in the future will be a big mess if we do re-generate the files in the PR. Maybe we should just configure the CI to regenerate everything after each PR merged to master.

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers please review.

@jmacd
Copy link
Contributor

jmacd commented Dec 5, 2019

Just to add a little more depth, I can imagine two scenarios calling for different formats, ideally. You might wish to export a list of timestamped values where each value has its own distinct timestamp. In that case, the original protocol would do. You might wish to elide the timestamp for a short time period by recording the values and a single timestamp, in which case a packed protobuf encoding would be desirable. Either of these could be added in the future, so I'm happy with this PR as it stands.

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers Can I get more reviews on this, please?

@bogdandrutu bogdandrutu merged commit 28a7413 into open-telemetry:master Dec 10, 2019
@tigrannajaryan
Copy link
Member Author

Thanks @yurishkuro

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.

Eliminate multi data point TimeSeries
4 participants