search

fivetran / dbt_intercom

Data models for Fivetran's Intercom connector built using dbt.
https://fivetran.github.io/dbt_intercom/
Apache License 2.0
4 stars 6 forks source link

Incorrect FIRST_CLOSE_AT on intercom__conversation_metrics #40

Closed alvarosiman closed 1 year ago

alvarosiman commented 1 year ago

Is there an existing issue for this?

Describe the issue

Please refer to ticket #125497 as it was the first reported issue we found with Fivetran's Intercom packages. After months of troubleshooting we finally found the issue with the package and Fivetran fixed.

From Intercom data we can clearly see that time of first close for the conversation with ID 38756 was 19:23 UTC but Fivetran's package is showing the next day at 22:08 UTC (the last close instead of the first). Could we please fix.

image

Relevant error log or model output

No response

Expected behavior

The expected behavior is to grab the first timestamp the conversations was closed

image

dbt Project configurations

Intercom

intercom_database: fivetran intercom_schema: intercom intercom__using_team: False intercom__using_company_tags: False

Package versions

packages:

dbt_date, dbt_expectations, codegen can all be upgraded

# but are blocked by github which has a dependency on
# dbt_utils [">=0.8.0", "<0.9.0"].

What database are you using dbt with?

snowflake

dbt Version

image

Additional Context

No response

Are you willing to open a PR to help address this issue?

fivetran-joemarkiewicz commented 1 year ago

Hi @alvarosiman thanks so much for opening this issue and raising the incorrect first close to our team.

When looking at our code to calculate first_close_at I see that we are grabbing the first created_at field for the filtered results in the cte where part_type = 'close' and author_type = 'admin'. Are you able to confirm if the close entry you are highlighting was done by an admin?

https://github.com/fivetran/dbt_intercom/blob/b31ee32fd908a519e96243c1c3c6cc1291c031bb/models/intermediate/int_intercom__conversation_part_events.sql#L12-L16

If it was not closed by an admin I would be curious to get your thoughts if that should still be counted as a first close time?

alvarosiman commented 1 year ago

Hi Joe, thanks for your reply!

I see, that's what I thought was happening. Based on how Fivetran packages treat Intercom data it would make sense to have a first_admin_close_at similar to the first_admin_response_at and last_admin_response_at

Intercom has many new functionalities, including very useful bots that can close conversations if an admin has responded and the end customer has been inactive after x amount of minutes. This is actually the case for most of our conversations. The user starts it, the admin responds and provides a solution or asks them a follow up question, but the end user doesn't respond again. Since having the admin close after responding would not make sense if the conversation isn't clearly finished, Intercom's bot asks the user if they are still there and closes the conversation telling them they can re-open it if they want to continue.

So in general it would be great to have a first_admin_close_at and a first_close_at regardless of if it was an admin

fivetran-joemarkiewicz commented 1 year ago

Thanks for the quick reply!

I was not aware of the bots Intercom may deploy to help automate these types of conversation parts (makes sense to me!). I would also agree with you that expanding the model to have two different versions of close_at can help users who leverage bots in their workflows. Would it be difficult on your end to manage these two fields when analyzing the data?

If not, then I feel this is an appropriate update to the package and can fold this into our upcoming sprint.

alvarosiman commented 1 year ago

I think there would be no issue on our side managing the two separate fields, it would make our analytics more accurate and give us more flexibility.

That would be great! When would the next sprint finish? And would we just have to update the package version to get access to this? Thanks!

fivetran-joemarkiewicz commented 1 year ago

Our next sprint begins Wednesday of next week. As this is likely a relatively small update we should be able to turn this around in the first half of the sprint.

Yup! Once the update is applied you will be able to install the newest version of the package to capture the changes. As we will likely be adding a new field and changing the definition of the first_close_at field, I would be most comfortable with this being a breaking change. So the next version will be v0.7.0.

Additionally, we will share a WIP branch here before releasing in case you wanted to validate on your end that changes look good before making the official release.

alvarosiman commented 1 year ago

Thanks Joe! When do you think we will we be able to validate on our side? and if it's taken on the first half of the sprint would it be released by the end of this week or the next?

fivetran-joemarkiewicz commented 1 year ago

Hi @alvarosiman I think conservatively we should have a working branch over to you to test by the end of this week. Realistically, this will be released early next week if all goes well during validation.

alvarosiman commented 1 year ago

Great, thanks for the update!

fivetran-joemarkiewicz commented 1 year ago

Hi @alvarosiman I wanted to post back that we currently have a working version of the package with the changes discussed earlier in this thread. If you have a moment, it would be great if you would be able to validate the changes on your end and confirm they make sense and are accurate.

You can test the changes by replacing your existing package dependency for intercom within your packages.yml with the following dependency:

packages:
  - git: https://github.com/fivetran/dbt_intercom.git
    revision: feature/close-at-updates
    warn-unpinned: false

You can see a full view of the updates applied in this branch by inspecting the notes and files changed within PR #41

Let me know if you have any questions. Thanks!

alvarosiman commented 1 year ago

Thanks @fivetran-joemarkiewicz

Will take a look and get back to you.

alvarosiman commented 1 year ago

Hi @fivetran-joemarkiewicz! Hope you are doing well. I checked a couple of runs and the information seems to be accurate. Thanks!

fivetran-joemarkiewicz commented 1 year ago

Brilliant! Thanks for checking. I will kick off the review process internally and will share once this is live.

alvarosiman commented 1 year ago

Hi @fivetran-joemarkiewicz hope everything is well! Is there an update on this PR? Do you think it will get released soon?

fivetran-joemarkiewicz commented 1 year ago

Hi @alvarosiman this is currently in the internal release process. I imagine you can expect this to be released today!

fivetran-joemarkiewicz commented 1 year ago

Hi @alvarosiman the changes from the branch I linked above are now live on the dbt hub! You can upgrade your package to the latest version (v0.7.0) in order to capture these changes.

As such, I will close this issue. Please feel free to reopen if the issue continues to persist on your end. Thanks again for helping work through this update!