Update Querying API docs based on additional feedback on #405 #410
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmontagu
changed the title
Deploying logfire-docs with
|
| Latest commit: |
7b6d6fc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4820e797.logfire-docs.pages.dev |
| Branch Preview URL: | https://dmontagu-address-samuel-feed.logfire-docs.pages.dev |
alexmojaki
reviewed
Sep 4, 2024
docs/guides/advanced/query_api.md
Outdated
| @@ -79,7 +79,7 @@ Here's an example of how to use these clients: | |||
| # Retrieve data in arrow format, and load into a polars DataFrame | |||
| # Note that JSON columns such as `attributes` will be returned as | |||
| # JSON-serialized strings | |||
| df_from_arrow = pl.from_arrow(await client.query_arrow(sql=query)) | |||
| df_from_arrow = pl.from_arrow(await client.query_arrow(sql=query)) # type: ignore | |||
There was a problem hiding this comment.
# type: ignore in docs????
alexmojaki
reviewed
Sep 4, 2024
docs/guides/advanced/query_api.md
Outdated
| - **`limit`**: An optional parameter to limit the number of rows returned by the query. If not specified, **the default limit is 500**. The maximum allowed value is 10,000. | ||
| - **`row_oriented`**: Only affects JSON responses. If set to `true`, the JSON response will be row-oriented; otherwise, it will be column-oriented. | ||
| - **`sql`**: The SQL query to execute. This is the only required query parameter. | ||
| - **`min_timestamp`**: An optional ISO-format timestamp to filter records with `start_timestamp` greater than this value for the `records` table or `recorded_timestamp` greater than this value for the `metrics` table. The same filtering can also be done manually within the query itself. |
There was a problem hiding this comment.
Does this now have a default that should be documented?
samuelcolvin
reviewed
Sep 6, 2024
docs/guides/advanced/query_api.md
Outdated
| @@ -197,22 +194,21 @@ else: | |||
|
|
|||
| ### Additional Configuration | |||
|
|
|||
| The Logfire API supports various query parameters and response formats to give you flexibility in how you retrieve your data: | |||
| The Logfire API supports various response formats and query parameters to give you flexibility in how you retrieve your data: | |||
|
|
|||
| - **Response Format**: Use the `Accept` header to specify the response format. Supported values include: | |||
There was a problem hiding this comment.
I don't think multiple levels of bullet point work in mkdocs by default.
There was a problem hiding this comment.
It seems to be working: https://4d3d712c.logfire-docs.pages.dev/guides/advanced/query_api/#additional-configuration
docs/guides/advanced/query_api.md
Outdated
| 'Authorization': f'Bearer {read_token}', | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| headers = {'Authorization': f'Bearer {read_token}'} |
There was a problem hiding this comment.
Are you sure we don't need the Accept header?
dmontagu
force-pushed
the
dmontagu/address-samuel-feedback
branch
from
d8fd6e1
to
943b604
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 131 131
Lines 9782 9782
Branches 1289 1289
=========================================
Hits 9782 9782 ☔ View full report in Codecov by Sentry. |
alexmojaki
reviewed
Sep 27, 2024
alexmojaki
reviewed
Sep 27, 2024
| @@ -197,22 +194,21 @@ else: | |||
|
|
|||
| ### Additional Configuration | |||
|
|
|||
| The Logfire API supports various query parameters and response formats to give you flexibility in how you retrieve your data: | |||
| The Logfire API supports various response formats and query parameters to give you flexibility in how you retrieve your data: | |||
There was a problem hiding this comment.
why were these swapped lol
There was a problem hiding this comment.
to match the order the bullets occur below
dmontagu
force-pushed
the
dmontagu/address-samuel-feedback
branch
from
4b6a121
to
7b6d6fc
Compare
petyosi
pushed a commit
that referenced
this pull request
Oct 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Address Samuel's feedback on #405.
I'll note that I am fairly confident the
Acceptheader is not required, but the default value sent by requests and various other tools is or contains*/*; I have a PR open on the platform in order to send JSON when the Accept header contains this.