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

make metric rest docs more consumable #454

Merged
2 commits merged into from
Nov 6, 2017
Merged

Conversation

chungg
Copy link
Member

@chungg chungg commented Oct 27, 2017

  • divide metric endpoint into crud sections

jd
jd previously approved these changes Oct 27, 2017
jd
jd previously approved these changes Oct 27, 2017
sileht
sileht previously approved these changes Oct 27, 2017
@chungg chungg dismissed stale reviews from sileht and jd via bbce50b October 27, 2017 21:10
@chungg chungg force-pushed the rest-split branch 2 times, most recently from bbce50b to 8374c47 Compare October 27, 2017 21:20
@@ -187,7 +222,7 @@ By default, new |measures| can only be processed if they have timestamps in the
future or part of the last aggregation period. The last aggregation period size
is based on the largest |granularity| defined in the |archive policy|
definition. To allow processing |measures| that are older than the period, the
`back_window` parameter can be used to set the number of coarsest periods to
`back_window` parameter can be used to set the number of historical periods to
Copy link
Member

Choose a reason for hiding this comment

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

Except you don't need the number of historical period.
We need to change that unit anyway. Logging a big.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not entirely sure what you mean by

We need to change that unit anyway. Logging a big.

you want me to change something in this patch?

Copy link
Member

Choose a reason for hiding this comment

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

s/big/bug/

I opened #459 to track that. I don't think your change here makes sense.
That does not mean I think the original text is easy to understand. But this line change remove information.
So it makes from hard to impossible to understand the parameter. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@chungg chungg force-pushed the rest-split branch 2 times, most recently from fe7d4ba to a4ba5b9 Compare October 29, 2017 18:09
jd
jd previously approved these changes Oct 30, 2017
can try to create them as long as an |archive policy| rule matches:

{{ scenarios['post-measures-batch-named-create']['doc'] }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Since that measures and aggregates have been distinguished now, maybe there should be a new section:

Aggregates
==========

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, i don't understand your comment. i already have an aggregates section below?

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point before, the incoming data points and the processed(aggregated) ones are both called measures. But since one of my commit and the discussion with jd, the processed ones are now called aggregates. To explain in another way, now we have 'measure storage', 'aggregate storage', 'index storage', and the Aggregates I mentioned is the one in 'aggregate' storage.
To make the document more consistent, measures can only be pushed(POST), and the ones get read(GET) are actually aggregates. So I hope that the Read part here should belong to Aggregates section rather than Measures section.
BTW, I know that for the endpoint, measures is still used when getting aggregates, but I'm planning to deprecate that and create a new aggregates endpoint. #421

Copy link
Member Author

Choose a reason for hiding this comment

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

so i understand the comment about terminology but i would probably have a discussion first about moving all GET actions to another endpoints. imo, it's a bad idea. people are use to CRUD concept and gnocch itself is already built on principle that everything is aggregated.

to me, aggregates endpoint is a custom-retrieval endpoint, not a simple read.

Copy link
Member

Choose a reason for hiding this comment

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

To circle back on this discussion, I lean toward @Asu4ni about terminology because I encountered a lot of people confused about measures vs. aggregates and having it clearer is better. The changes proposed in #421 are orthogonal to this though – they could happen one day, but for now we should just focus on what exists. :)

That being said, that seems good enough for me now. The doc really talks about retrieving aggregates and not measures. :)

The list of points returned is composed of tuples with (timestamp,
|granularity|, value) sorted by timestamp. The |granularity| is the |timespan|
covered by aggregation for this point.
Filter
Copy link
Contributor

Choose a reason for hiding this comment

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

I consider Filter should be the subsection of Read since it is used when reading aggregates

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i considered this. i really wanted filter to be top level but ok.

@@ -119,24 +162,25 @@ timestamp:

{{ scenarios['get-measures-from']['doc'] }}

Aggregate
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Aggregation is better. not to be confused with 'aggregates'. and the parameter in url uses aggregation as well

Copy link
Member Author

Choose a reason for hiding this comment

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

sure.


.. _aggregates:

Aggregates: on the fly, measurements modification and aggregation
===================================================================
Aggregates
Copy link
Contributor

Choose a reason for hiding this comment

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

on the fly should be retained. Otherwise, it'll be confused with the stored Aggregates above.

Copy link
Member Author

Choose a reason for hiding this comment

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

how about dynamic aggregates? 'on the fly' is more slang i feel.

- divide metric section into crud tasks
- divide measures section into crud tasks
- divide archive-policy section into crud tasks
- divide archive-policy-rule section into crud tasks
- divide resources section into crud tasks
- divide resource-type section into crud tasks

- create common search section
  - create anchors for each functionality
- better highlight aggregates API

- remove redundant create-resource-with-new-metrics example
can try to create them as long as an |archive policy| rule matches:

{{ scenarios['post-measures-batch-named-create']['doc'] }}

Copy link
Member

Choose a reason for hiding this comment

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

To circle back on this discussion, I lean toward @Asu4ni about terminology because I encountered a lot of people confused about measures vs. aggregates and having it clearer is better. The changes proposed in #421 are orthogonal to this though – they could happen one day, but for now we should just focus on what exists. :)

That being said, that seems good enough for me now. The doc really talks about retrieving aggregates and not measures. :)

@ghost ghost merged commit b705a8d into gnocchixyz:master Nov 6, 2017
@chungg chungg deleted the rest-split branch November 6, 2017 19:09
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants