Skip to content

feat: Add traffic streams#193

Merged
edgarrmondragon merged 18 commits intoMeltanoLabs:mainfrom
pulumi:add-traffic-streams
May 5, 2023
Merged

feat: Add traffic streams#193
edgarrmondragon merged 18 commits intoMeltanoLabs:mainfrom
pulumi:add-traffic-streams

Conversation

@sicarul
Copy link
Copy Markdown
Contributor

@sicarul sicarul commented Apr 19, 2023

This PR adds the Traffic streams to collect how much traffic did we have on each Repository.

It'll only collect data for the repositories the token has write access to, and will log an info message for those that it couldn't collect.

@sicarul sicarul changed the title Add traffic streams feat: Add traffic streams Apr 19, 2023
@sicarul sicarul requested a review from edgarrmondragon April 19, 2023 18:59
@sicarul sicarul marked this pull request as draft April 19, 2023 19:09
@sicarul sicarul marked this pull request as ready for review April 19, 2023 20:27
@sicarul
Copy link
Copy Markdown
Contributor Author

sicarul commented Apr 20, 2023

It seems the test failed:


-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tap_github/tests/test_tap.py::test_get_a_repository_in_repo_list_mode[False] - singer_sdk.exceptions.FatalAPIError: 403 Client Error: b'{"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/metrics/traffic#get-repository-clones"}' (Reason: Forbidden) for path: /repos/MeltanoLabs/tap-github/traffic/clones
FAILED tap_github/tests/test_tap.py::test_get_a_repository_in_repo_list_mode[True] - singer_sdk.exceptions.FatalAPIError: 403 Client Error: b'{"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/metrics/traffic#get-repository-clones"}' (Reason: Forbidden) for path: /repos/MeltanoLabs/tap-github/traffic/clones
FAILED tap_github/tests/test_tap.py::test_last_state_message_is_valid - singer_sdk.exceptions.FatalAPIError: 403 Client Error: b'{"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/metrics/traffic#get-repository-clones"}' (Reason: Forbidden) for path: /repos/MeltanoLabs/tap-github/traffic/clones
============= 3 failed, 7 passed, 714 warnings in 84.81s (0:01:24) =============
Error: Process completed with exit code 1.

I think it's because of a permissions issue on the user running the test.

@edgarrmondragon
Copy link
Copy Markdown
Member

It seems the test failed:


-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tap_github/tests/test_tap.py::test_get_a_repository_in_repo_list_mode[False] - singer_sdk.exceptions.FatalAPIError: 403 Client Error: b'{"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/metrics/traffic#get-repository-clones"}' (Reason: Forbidden) for path: /repos/MeltanoLabs/tap-github/traffic/clones
FAILED tap_github/tests/test_tap.py::test_get_a_repository_in_repo_list_mode[True] - singer_sdk.exceptions.FatalAPIError: 403 Client Error: b'{"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/metrics/traffic#get-repository-clones"}' (Reason: Forbidden) for path: /repos/MeltanoLabs/tap-github/traffic/clones
FAILED tap_github/tests/test_tap.py::test_last_state_message_is_valid - singer_sdk.exceptions.FatalAPIError: 403 Client Error: b'{"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/metrics/traffic#get-repository-clones"}' (Reason: Forbidden) for path: /repos/MeltanoLabs/tap-github/traffic/clones
============= 3 failed, 7 passed, 714 warnings in 84.81s (0:01:24) =============
Error: Process completed with exit code 1.

I think it's because of a permissions issue on the user running the test.

@sicarul Yeah, can you add note to the readme documenting which permissions are required to pull the traffic streams?

@sicarul
Copy link
Copy Markdown
Contributor Author

sicarul commented Apr 23, 2023

Added @edgarrmondragon !

@sicarul
Copy link
Copy Markdown
Contributor Author

sicarul commented Apr 26, 2023

@edgarrmondragon do we need anything else to move this forward?

@edgarrmondragon
Copy link
Copy Markdown
Member

@edgarrmondragon do we need anything else to move this forward?

@sicarul Tests are still failing so I worry that this may cause similar failures to users of the tap if their tokens don't support pulling from the traffic endpoints.

Wdyt about adding a tap setting (e.g. sync_traffic_streams) that by default makes the tap omit these streams?

cc @kgpayne if you have other ideas

@sicarul
Copy link
Copy Markdown
Contributor Author

sicarul commented Apr 27, 2023

@edgarrmondragon do we need anything else to move this forward?

@sicarul Tests are still failing so I worry that this may cause similar failures to users of the tap if their tokens don't support pulling from the traffic endpoints.

Wdyt about adding a tap setting (e.g. sync_traffic_streams) that by default makes the tap omit these streams?

cc @kgpayne if you have other ideas

Maybe i could also handle this error instead?
“ Resource not accessible by integration”

However, i do not believe any user would try to run this entire tap without selecting streams, some are monstruous.

@edgarrmondragon
Copy link
Copy Markdown
Member

edgarrmondragon commented Apr 28, 2023

However, i do not believe any user would try to run this entire tap without selecting streams, some are monstruous.

@sicarul You're right, but the SDK tries to be helpful to folks just getting started with singer.io and so all streams are selected by default in the default catalog.

However, we could change that in your streams. I'll propose some changes.

(I also started a discussion in the SDK: meltano/sdk#1652)

sicarul and others added 3 commits May 3, 2023 11:53
Co-authored-by: Edgar R. M. <edgarrm358@gmail.com>
Co-authored-by: Edgar R. M. <edgarrm358@gmail.com>
Co-authored-by: Edgar R. M. <edgarrm358@gmail.com>
@edgarrmondragon edgarrmondragon self-requested a review May 3, 2023 18:51
@edgarrmondragon
Copy link
Copy Markdown
Member

@sicarul this LGTM! Just a minor comment on the readme and this is good to merge for me 🙂

@edgarrmondragon edgarrmondragon requested a review from laurentS May 3, 2023 18:55
Co-authored-by: Edgar R. M. <edgarrm358@gmail.com>
@edgarrmondragon
Copy link
Copy Markdown
Member

@sicarul Can you update your branch from main?

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented May 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@edgarrmondragon
Copy link
Copy Markdown
Member

Thanks @sicarul!

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.

3 participants