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

feat: FileInputStream/FileOutputStream instrumentation #239

Merged
merged 12 commits into from
Dec 14, 2021

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Nov 25, 2021

#skip-changelog

📜 Description

  • Introduce new screen in the sample where we utilise File I/O apis
  • Introduce new remapping

image

💡 Motivation and Context

💚 How did you test it?

Manually for now - automated tests will follow in future PRs

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

The PR is targeting integration branch for now, the next steps are:

  • Runtime instrumentation lib with the actual instrumented classes
  • More classes instrumentation (FileReader/FileWriter, context.openFileInput/Output)
  • Tests

@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2021

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 305cb2e

override fun isInstrumentable(data: ClassContext): Boolean {
return when {
data.currentClassData.className.startsWith("io.sentry")
&& !data.currentClassData.className.startsWith("io.sentry.android.roomsample") -> false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just for the sample to work, probably shouldn't be in the production code.. maybe worth introducing ignore list config in the gradle plugin, where users can specify packagenames to ignore?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but I'd rather take it as feature request, not really part of this PR.
we could rename the sample package if necessary to avoid this.
on Java, we special case these packages btw https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/SentryStackTraceFactory.java#L41-L46
because of our samples and mobile app.

@marandaneto
Copy link
Contributor

why #skip-changelog? :)

@romtsn
Copy link
Member Author

romtsn commented Nov 26, 2021

why #skip-changelog? :)

targeting integration branch for now

@romtsn
Copy link
Member Author

romtsn commented Dec 14, 2021

@marandaneto PTAL. We've changed approach so this

val fis = FileInputStream("Test.txt")

Turns into this

val fis = SentryFileInputStream.Factory.create(FileInputStream("Test.txt"), "Test.txt")

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@romtsn romtsn merged commit 7f1e3dd into feat/file-io Dec 14, 2021
@romtsn romtsn deleted the feat/file-io-stream-instr branch December 14, 2021 12:45
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.

2 participants