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

Add logs listing feature #622

Merged

Conversation

userlocalhost
Copy link
Contributor

@userlocalhost userlocalhost commented Jan 16, 2021

Requirements for Contributing to this repository

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must only fix one issue, or add one feature, at the time.
  • The pull request must update the test suite to demonstrate the changed functionality.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see CONTRIBUTING.

What does this PR do?

This PR implement a feature which is mentioned at #611.

Description of the Change

This PR implemented API wrapper of 'logs-queries/list' (c.f.
https://docs.datadoghq.com/api/v1/logs/) that returns list of logs which
are matched with specified log query.

Alternate Designs

This PR follows implementation of other API wrappers.

Possible Drawbacks

Yes. This PR has backward compatibility because this just add a new API wrapper.

Verification Process

This is an example code to get logs by using added implementation. If you run this code, please replace APP_KEY, API_KEY and SEARCH_QUERY for your environment.

from datadog import api, initialize

options = {
    'api_key': 'APP_KEY',
    'app_key': 'API_KEY',
}

initialize(**options)

resp = api.Logs.list({
    'time': {
        "from": "2021-01-14T00:00:00Z",
        "to": "2021-01-15T00:00:00Z"
    },
    'query': 'SEARCH_QUERY',
    'limit': 10,
})
print(resp)

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

This commit implemented API wrapper of 'logs-queries/list' (c.f.
https://docs.datadoghq.com/api/v1/logs/) that returns list of logs which
are mached with specified log query.
@userlocalhost userlocalhost requested a review from a team as a code owner January 16, 2021 14:33
@therve
Copy link
Contributor

therve commented Jan 16, 2021

Hi,

This is already handled by our generated client: https://github.com/DataDog/datadog-api-client-python/blob/master/docs/v1/LogsApi.md#list_logs

Would that work for your purpose?

@userlocalhost
Copy link
Contributor Author

userlocalhost commented Jan 17, 2021

@therve
Thank you for your prompt reply. Yes, I confirmed I could do that using the datadog-api-client-python.

But I'm not only interested in using this feature, but I want to use a software that depends on datadogpy.
That's the reason why I made this PR that implements API wrapper of logs-queries.

BTW, I wonder why Datadog team develops two different Python libraries. Will this (datadogpy) not be extended any more and be obsoleted?

@therve
Copy link
Contributor

therve commented Jan 17, 2021

BTW, I wonder why Datadog team develops two different Python libraries.

It's tough to keep up manually with all the APIs we produce, so we started a new generated library.

Will this (datadogpy) not be extended any more and be obsoleted?

datadogpy will continue to live, as it contains other capabilities (like metrics and dogstatsd) that our generated library doesn't do. We're still trying to define what our strategy is going to be, but we'll most likely won't add new endpoints to datadogpy like this one. One possibility would be to depend on datadog-api-client-python and expose the functionality that way, would that work for you?

@userlocalhost
Copy link
Contributor Author

userlocalhost commented Jan 17, 2021

Thank you for answering my question. May I ask some more questions?

I wonder what we should do in the use-case that datadog user wants to use both logs and metrics API simultaneously(e.g. posting metric that is made from collected logs). Should we have to use both datadogpy and datadog-api-client-python libraries in our software?

And as you said, the datadog-api-client-python seems not to have implementation to handle some metrics APIs (e.g. [POST] /api/v1/series).
Will you add features that handle those endpoints to datadog-api-client-python?

@therve
Copy link
Contributor

therve commented Jan 17, 2021

Thank you for answering my question. May I ask some more questions?

I wonder what we should do in the use-case that datadog user wants to use both logs and metrics API simultaneously(e.g. posting metric that is made from collected logs). Should we have to use both datadogpy and datadog-api-client-python libraries in our software?

Today, yes. As mentioned previously, one possible implementation is to import datadog-api-client-python in datadogpy, so that you only depend and use datadogpy, but it uses datadog-api-client-python under the hood. Today you have to use both.

And as you said, the datadog-api-client-python seems not to have implementation to handle some metrics APIs (e.g. [POST] /api/v1/series).
Will you add features that handle those endpoints to datadog-api-client-python?

No we don't plan to add metric submission to generated clients.

@userlocalhost
Copy link
Contributor Author

userlocalhost commented Jan 17, 2021

Today, yes. As mentioned previously, one possible implementation is to import datadog-api-client-python in datadogpy, so that you only depend and use datadogpy, but it uses datadog-api-client-python under the hood. Today you have to use both.

Thank you for your reply. Sorry, I'm not sure the meaning of "Today". If you don't plan to support metrics API on the datadog-api-client-python. User permanently has to use both libraries for that use-case, doesn't it?

Or, is there possibility to consider to merge a PR when I make another PR that implement API handler of metrics on the datadog-api-client-python like this?
I'm a fan of Datadog so I'm willing to contribute to that Datadog libraries to be better.

@therve
Copy link
Contributor

therve commented Jan 17, 2021

Thank you for your reply. Sorry, I'm not sure the meaning of "Today". If you don't plan to support metrics API on the datadog-api-client-python. User permanently has to use both libraries for that use-case, doesn't it?

User has to install both, but we can make it so that you only explicitly depend on and import datadogpy.

Or, is there possibility to consider to merge a PR when I make another PR that implement API handler of metrics on the datadog-api-client-python like this?

No we won't accept such a contribution. We might at some point add it, but it would be generated anyway.

@userlocalhost
Copy link
Contributor Author

userlocalhost commented Jan 18, 2021

Sorry, I had insufficient understanding about what the difference between datadogpy and datadog-api-client-python.

To summarize, the datadogpy is a traditional datadog python library and it has been extended in manual. On the other hand, the datadog-api-client-python is another python API client for the Datadog API. Thats code is generated using some other tools. And it is expected to collect all public Datadog API endpoints, but it has not finished it yet, right?

I think the datadog-api-client-python is elaborate and cool solution. And I also understood the reason why it won't accept this type of in manual extended because that code would be generated automatically.

It's tough to keep up manually with all the APIs we produce

we'll most likely won't add new endpoints to datadogpy

I know that feeling, but I suspect whether this bold decision is good for Datadog users. I suggest to keep maintaining (including extending features) datadogpy until the datadog-api-client-python has been completed. Following is reasons why I think so.

(reason1) It causes an inconvenience for users

If the datadog-api-client-python has already cover all the Datadog API, I'll be all for that decision. But it hasn't yet. If you make a decision that datadogpy refuse any kind extension any more, user might feel that "Both official python libraries are useless. One has abandoned extending of features, some API endpoints have not been supported. And another is in the middle of development, some API endpoints also have not been supported yet".
(I don't mean to offend you, but this is an honest opinion from an user perspective)

User carefully has to distinguish which library should we use depending on what API endpoints user want to access. This is inconvenient for users I think.

(reason2) Probably no one knows it

I've read the whole datadogpy README and an official documentation that describes client libraries. There is no explanation that the datadogpy intends not to develop extensions and accept any PR that extends feature any more. This is against for users expectation I think.


I understand how you feel that you want to dedicate to developing the datadog-api-client-python. But please at least accept contributions for the datadogpy from user if it's worth for users until development of the datadog-api-client-python is finished.

I hope Datadog team makes a decision that makes Datadog users happy.

Sincerely,

@therve
Copy link
Contributor

therve commented Jan 18, 2021

I didn't say we won't accept your patch. I said we already have a library which does what you need. If the plan isn't clear it's because it's not finalized. We'll clarify the place of both libraries when it is. Thanks.

@therve
Copy link
Contributor

therve commented Jan 21, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@therve therve added the changelog/Added Added features results into a minor version bump label Jan 21, 2021
@therve therve changed the title Implemented feature to get logs Add logs listing feature Jan 21, 2021
@therve therve merged commit ca86a40 into DataDog:master Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants