🎉 New Source: Sentry #6975

koji-m · 2021-10-12T13:24:13Z

What

Add new source Sentry
Participating in hacktoberfest-2021 Source: Sentry connector-contest#9
This PR closes New Source: Sentry #5692

How

Added support for the following streams:

Events
Issues
ProjectDetail (for check_connection only)

Test Runs

Integration Test

Unit Test

Pre-merge Checklist

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

CLAassistant · 2021-10-12T13:24:17Z

All committers have signed the CLA.

harshithmullapudi · 2021-10-13T16:55:11Z

@koji-m I will take a look at it tomorrow.

harshithmullapudi · 2021-10-15T03:12:41Z

airbyte-integrations/connectors/source-sentry/source_sentry/source.py

+            "organization": config["organization"],
+            "project": config["project"],
+        }
+        return [Events(**stream_args), Issues(**stream_args)]


ProjectDetail is not added here ?

I've added ProjectDetail and Projects.

harshithmullapudi · 2021-10-15T03:13:25Z

airbyte-integrations/connectors/source-sentry/source_sentry/source.py

+            project_detail_stream = ProjectDetail(
+                authenticator=TokenAuthenticator(token=config["auth_token"]),
+                hostname=config.get("hostname", self.DEFAULT_HOST),
+                organization=config["organization"],
+                project=config["project"],
+            )
+            next(project_detail_stream.read_records(sync_mode=SyncMode.full_refresh))


Is there any simple way to test if the token is valid ? Which is more generic ?

Yes, I added Projects stream for endpoint /api/0/projects/ and changed to use Projects stream to test if the token is valid.

harshithmullapudi · 2021-10-15T03:14:58Z

airbyte-integrations/connectors/source-sentry/source_sentry/streams.py

+        if "next" in response.links and "results" in response.links["next"] and "cursor" in response.links["next"]:
+            if response.links["next"]["results"] == "true":
+                return {"cursor": response.links["next"]["cursor"]}
+        return None


can we use try catch here and comment here on what can go wrong

I fixed to handle errors with try-catch, and also added comment.

harshithmullapudi · 2021-10-15T03:18:41Z

airbyte-integrations/connectors/source-sentry/source_sentry/streams.py

+
+
+class Events(SentryStream):
+    primary_key = "id"


you can keep this for SentryStream

I've moved primary_key variable to SentryStream class.

harshithmullapudi · 2021-10-15T03:19:24Z

Also can you run ./gradlew format

koji-m · 2021-10-15T16:45:44Z

I've ran ./gradlew format.

harshithmullapudi · 2021-10-18T06:21:18Z

airbyte-integrations/connectors/source-sentry/source_sentry/schemas/project_detail.json

+      "required": [
+        "avatar",
+        "dateCreated",
+        "id",
+        "isEarlyAdopter",
+        "name",
+        "require2FA",
+        "slug",
+        "status"
+      ],


can we remove these required keys

harshithmullapudi · 2021-10-18T06:21:27Z

airbyte-integrations/connectors/source-sentry/source_sentry/schemas/project_detail.json

+      "required": [
+        "authors",
+        "commitCount",
+        "data",
+        "dateCreated",
+        "dateReleased",
+        "deployCount",
+        "firstEvent",
+        "lastCommit",
+        "lastDeploy",
+        "lastEvent",
+        "newGroups",
+        "owner",
+        "projects",
+        "ref",
+        "shortVersion",
+        "url",
+        "version"
+      ],


can we remove these required keys

harshithmullapudi · 2021-10-18T06:21:56Z

airbyte-integrations/connectors/source-sentry/source_sentry/schemas/project_detail.json

+  "required": [
+    "allowedDomains",
+    "avatar",
+    "color",
+    "dataScrubber",
+    "dataScrubberDefaults",
+    "dateCreated",
+    "defaultEnvironment",
+    "digestsMaxDelay",
+    "digestsMinDelay",
+    "features",
+    "firstEvent",
+    "hasAccess",
+    "id",
+    "isBookmarked",
+    "isInternal",
+    "isMember",
+    "isPublic",
+    "latestRelease",
+    "name",
+    "options",
+    "organization",
+    "platform",
+    "platforms",
+    "processingIssues",
+    "relayPiiConfig",
+    "resolveAge",
+    "safeFields",
+    "scrapeJavaScript",
+    "scrubIPAddresses",
+    "securityToken",
+    "securityTokenHeader",
+    "sensitiveFields",
+    "slug",
+    "status",
+    "storeCrashReports",
+    "subjectPrefix",
+    "subjectTemplate",
+    "team",
+    "teams",
+    "verifySSL"
+  ],


can we remove these required keys. Same across the schemas

I removed required-key fields from all of the schemas.

harshithmullapudi · 2021-10-18T06:23:33Z

airbyte-integrations/connectors/source-sentry/source_sentry/source.py

+
+# Source
+class SourceSentry(AbstractSource):
+    DEFAULT_HOST = "sentry.io"


Is this needed here since we have already used default in the config? can't we directly get this from config?

We don't need to set default value in code. I've fixed.

marcosmarxm · 2021-10-20T02:39:03Z

airbyte-integrations/connectors/source-sentry/source_sentry/streams.py

+        except KeyError:
+            return None


any change with this we enter a loop because is not possible to update the cursor? If it's expected to always have the values raise an error.

I didn't update the cursor parameter in the request_params method, so I fixed it to do so.
And as it's expected to always have the results and cursor values in link header, I fixed to raise error if there is no values for that.

marcosmarxm · 2021-10-20T02:41:46Z

airbyte-integrations/connectors/source-sentry/source_sentry/streams.py

+class Events(SentryStream):
+    def __init__(self, organization: str, project: str, **kwargs):


Please add docs to each endpoint, see example:

airbyte/airbyte-integrations/connectors/source-cart/source_cart/streams.py

Lines 104 to 108 in 1fcd6c4

class CustomersCart(IncrementalCartStream):

"""

Docs: https://developers.cart.com/docs/rest-api/restapi.json/paths/~1customers/get

"""

I've added docs to each endpoint.

marcosmarxm

small comments, but overall great work! @harshithmullapudi execute tests and makes sure it's passing CI (format and tests)

avida · 2021-10-29T10:23:23Z

would lean towards simplification here and remove the need for the xWithHash() classes entirely by refactoring serialize() like so:

def serialize(value) -> str: 
     """
     Serialize python object composed of primitive types.
     The resultant serialized string is order agnostic, such that the ordering of dicts and lists at any level of nesting doesn't affect the return value as long as data content is equal.
     """ 
     if isinstance(value, Mapping): 
         return json.dumps({k: serialize(v) for k, v in value.items()}, sort_keys=True)
     if isinstance(value, List): 
         return json.dumps(sorted([serialize(v) for v in value]))
     return str(value)

That's how it was before Ive replaced it with those xWithHash classes to correctly log records with detailed_logger fixture. Changing it back would break

Do we rely on differentiation in order of lists at all that this would break?

No, different order records should not fail test. We had such defect in SAT #5735 but it was fixed .

If the order in the list of response makes sense, my change suggestion is invalid. If so, I have to think of other ways

No, it should work with any items order.

@koji-m you can find acceptance_tests_logs directory after SAT run with detaild log on each case. For test_sequential_reads you gonna need add those lines to print records from first and second run: https://github.com/airbytehq/airbyte/pull/7440/files#diff-9578c930b7e368f14eaeccbae67aa1808f87f2ea4d1b1d8202694fd6cda1348dR25

Dont forget to rebuild SAT image or just run SAT with command line: python -m pytest -p source_acceptance_test.plugin

Could you try it out and share detailed logs with failed case?

avida · 2021-10-29T11:04:12Z

@koji-m Created an issue #7487 will work on it

Phlair · 2021-10-29T12:08:34Z

That's how it was before Ive replaced it with those xWithHash classes to correctly log records with detailed_logger fixture. Changing it back would break

@avida why would this break logging records? I must be missing some understanding there (pointing me at relevant PR/code might be easiest answer 😄)

avida · 2021-10-29T13:06:43Z

@avida why would this break logging records? I must be missing some understanding there (pointing me at relevant PR/code might be easiest answer smile)

@Phlair there is function in detailed_logger that dumps well formatted json into log:

logger.info(json.dumps(list(l), indent=1))

serialized raw string would be a pain to read or parse with diff tools. Btw Ive created PR #7491 where I replaced serialize function (cause we actually dont need to serialize objects, just to compare) . Please check it out.

koji-m · 2021-10-29T13:19:02Z

@avida Thank you for your advice and how to isolate the problem. I tried to confirm difference between first and second read. But response data is so large to compare, so before that I tried small test as follow:

import json
import functools
from typing import List, Mapping

@functools.total_ordering
class DictWithHash(dict):

    _hash: str = None

    def __hash__(self):
        if not self._hash:
            self._hash = hash(json.dumps({k: serialize(v) for k, v in self.items()}, sort_keys=True))
        return self._hash

    def __lt__(self, other):
        return hash(self) < hash(other)

    def __eq__(self, other):
        return hash(self) == hash(other)

def serialize(value) -> str:
    """Simplify comparison of nested dicts/lists"""
    if isinstance(value, Mapping):
        return DictWithHash(value)
    if isinstance(value, List):
        return sorted([serialize(v) for v in value])
    return str(value)


first = {
    "organization": {
        "features": [
              "performance-tag-page",
              "issue-percent-filters",
        ]
   }
}

second = {
    "organization": {
        "features": [
             "issue-percent-filters",
             "performance-tag-page",
         ]
   }
}

print(f'first {hash(serialize(first))}')
print(f'second {hash(serialize(second))}')

This show different hash value as follow:

first 2562171057633847383
second 4625636057198759702

I think the two values must be the same. There may be some mistakes in the content being verified, but I would be grateful if you could confirm it.

avida · 2021-10-29T13:23:28Z

@koji-m Yes, it should be the same. Please check out my PR #7491, Ive added your case to unittests

koji-m · 2021-10-29T13:59:24Z

I checked the test case, it looks good to me.

keu · 2021-10-29T14:01:27Z

ok, to add to everything above, the original serialize didn't have this issue and was using simple technic that most of pytest plugin use to compare, serialize and pprint will show much better diff than raw dicts. @avida added file logger that supposed to be used manually for detailed investigation, that is why objects there is not serialized.

harshithmullapudi · 2021-11-01T09:54:44Z

@koji-m is this resolved and are test cases passing in local ?

koji-m · 2021-11-01T12:39:49Z

Not yet resolved, but if #7491 is merged, I would assume the test case will pass. I'll let you know when the test is complete.

avida · 2021-11-01T13:53:34Z

@koji-m #7491 merged, please check it out

koji-m · 2021-11-01T14:48:08Z

Thank you @avida .
@harshithmullapudi I merged master and passed source-acceptance-test in local. Would you check it out again?

karinakuz · 2021-11-02T13:31:59Z

Examples of issue and event are added, tested APIs for:

evens https://sentry.io/api/0/projects/airbyte-09/airbyte-09/events/
issues https://sentry.io/api/0/projects/airbyte-09/airbyte-09/issues/
project details https://sentry.io/api/0/projects/airbyte-09/airbyte-09/

unity · 2021-11-02T15:08:37Z

OneSignal is now a duplicate source, prevents usage.

airbyte/airbyte-config/init/src/main/resources/seed/source_definitions.yaml

Line 604 in 3928c0c

- sourceDefinitionId: bb6afd81-87d5-47e3-97c4-e2c2901b1cf8

airbyte/airbyte-config/init/src/main/resources/seed/source_definitions.yaml

Line 355 in 3928c0c

- name: OneSignal

airbyte-server      | 2021-11-02 15:10:39 ERROR i.a.s.e.UncaughtExceptionMapper(toResponse):22 - {workspace_app_root=/tmp/workspace/server/logs} - Uncaught exception
airbyte-server      | java.lang.IllegalArgumentException: Multiple records have the name: OneSignal

koji-m · 2021-11-02T22:47:06Z

Sorry for the omission in the confirmation when merging master into this PR. I'll thoroughly check for conflicts.
#7565

* add Events, Issues, ProjectDetail stream * add P/R number * add SUMMARY entry * add docs/integrations/README.md entry * add source_definitions.yaml entry * add connector JSON definition * add builds.md entry * SentryStream keeps primary_key * add Projects stream * change stream for connection checking * handling errors with try-catch in next_page_token function * remove required key field from schemas * remove DEFAULT_HOST * raise error if link header don't have mandatory field * fix unit test for streams * update cursor for pagination * add docs to each endpoint * add hostname property to invalid_config * fix schema * add hostname to sample_config

add Events, Issues, ProjectDetail stream

644ce05

github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Oct 12, 2021

koji-m added 5 commits October 12, 2021 23:41

add P/R number

d9cbd33

add SUMMARY entry

32ab8b8

add docs/integrations/README.md entry

6e4416c

add source_definitions.yaml entry

c10f42b

add connector JSON definition

f11264f

avaidyanatha added the community_pr label Oct 12, 2021

add builds.md entry

5b9e65c

marcosmarxm added community hacktober labels Oct 13, 2021

marcosmarxm assigned harshithmullapudi Oct 13, 2021

marcosmarxm removed the community_pr label Oct 14, 2021

harshithmullapudi suggested changes Oct 15, 2021

View reviewed changes

koji-m added 4 commits October 15, 2021 21:44

SentryStream keeps primary_key

fc196db

add Projects stream

6cf7286

change stream for connection checking

d2d8a02

handling errors with try-catch in next_page_token function

5e81cad

harshithmullapudi suggested changes Oct 18, 2021

View reviewed changes

koji-m added 2 commits October 18, 2021 22:40

remove required key field from schemas

a4369fa

remove DEFAULT_HOST

ee506eb

marcosmarxm reviewed Oct 20, 2021

View reviewed changes

marcosmarxm requested changes Oct 20, 2021

View reviewed changes

raise error if link header don't have mandatory field

c2333c0

avida mentioned this pull request Oct 29, 2021

SAT not works correctly on records with nested structure and different orders #7487

Closed

Merge branch 'master' into 5692-new-source-sentry

af4d1d3

harshithmullapudi mentioned this pull request Nov 1, 2021

added credentials to ci for new source sentry #7335

Merged

38 tasks

add hostname to sample_config

a0a9b4a

harshithmullapudi merged commit c689594 into airbytehq:master Nov 2, 2021

This was referenced Nov 2, 2021

require PRs to pass against master before getting merged #7566

Closed

don't fail listing definitions if the format from the server is invalid #7567

Closed

koji-m deleted the 5692-new-source-sentry branch November 3, 2021 03:24

jrhizor mentioned this pull request Nov 3, 2021

Bump Airbyte version from 0.30.25-alpha to 0.30.26-alpha #7599

Merged

marcosmarxm removed the hacktober label Oct 20, 2022

octavia-squidington-iv added bounty connectors/source/sentry labels Oct 20, 2022

		class Events(SentryStream):
		def __init__(self, organization: str, project: str, **kwargs):

	class CustomersCart(IncrementalCartStream):
	"""
	Docs: https://developers.cart.com/docs/rest-api/restapi.json/paths/~1customers/get
	"""

🎉 New Source: Sentry #6975

🎉 New Source: Sentry #6975

Conversation

koji-m commented Oct 12, 2021 • edited Loading

What

How

Recommended reading order

Test Runs

Integration Test

Unit Test

Pre-merge Checklist

Community member or Airbyter

Airbyter

CLAassistant commented Oct 12, 2021 • edited Loading

harshithmullapudi commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshithmullapudi commented Oct 15, 2021

koji-m commented Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koji-m Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosmarxm left a comment

Choose a reason for hiding this comment

avida commented Oct 29, 2021

avida commented Oct 29, 2021

Phlair commented Oct 29, 2021

avida commented Oct 29, 2021

koji-m commented Oct 29, 2021

avida commented Oct 29, 2021

koji-m commented Oct 29, 2021

keu commented Oct 29, 2021 • edited Loading

harshithmullapudi commented Nov 1, 2021

koji-m commented Nov 1, 2021

avida commented Nov 1, 2021

koji-m commented Nov 1, 2021

karinakuz commented Nov 2, 2021

unity commented Nov 2, 2021 • edited Loading

koji-m commented Nov 2, 2021 • edited Loading

koji-m commented Oct 12, 2021 •

edited

Loading

CLAassistant commented Oct 12, 2021 •

edited

Loading

koji-m commented Oct 15, 2021 •

edited

Loading

koji-m Oct 18, 2021 •

edited

Loading

keu commented Oct 29, 2021 •

edited

Loading

unity commented Nov 2, 2021 •

edited

Loading

koji-m commented Nov 2, 2021 •

edited

Loading