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.

Sign up for GitHub

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

Jump to bottom

[Sessions] Add SessionStartEvent wrapper for building the nanopb proto #10311

Merged
merged 5 commits into from
Oct 19, 2022
Merged

[Sessions] Add SessionStartEvent wrapper for building the nanopb proto #10311

merged 5 commits into from
Oct 19, 2022

Conversation

samedson
Copy link
Contributor

@samedson samedson commented Oct 6, 2022 •
edited
Loading

  • Adds functions for interacting with nanopb. Some parts of this were difficult to do in Swift, so I had to do them in ObjC.
  • Adds SessionStartEvent as an abstraction over the proto
  • Adds FireLog dependency, and logs events to FireLog

#no-changelog

@samedson samedson added the sessions Changes pertaining to the Firebase Sessions SDK label Oct 6, 2022
@samedson samedson force-pushed the sessions-proto2 branch 2 times, most recently from 6433831 to e0f4b83 Compare October 17, 2022 14:58
@samedson samedson changed the title [Sessions] Add nanopb integration and a SessionStartEvent wrapper [Sessions] Add SessionStartEvent wrapper for nanopb Oct 17, 2022
@samedson samedson changed the title [Sessions] Add SessionStartEvent wrapper for nanopb [Sessions] Add SessionStartEvent wrapper for building the nanopb proto Oct 17, 2022
@samedson samedson requested a review from ncooke3 October 17, 2022 19:45
@firebase firebase deleted a comment from google-oss-bot Oct 17, 2022
@samedson samedson force-pushed the sessions-proto2 branch 2 times, most recently from 3b1af0a to f75bc8f Compare October 18, 2022 14:33
visumickey
visumickey reviewed Oct 18, 2022
View reviewed changes
Copy link
Contributor

@visumickey visumickey left a comment

Choose a reason for hiding this comment

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

Apart from the comments: 3 major suggestions:

  1. We need a clear/module class interaction defined and name the classes according to them. Some generic terms used for classes would grow in time to become hard to maintain.
  2. Lets add comments to every class/function/variable to ensure that it becomes easier to understand that the class/function is supposed to do. Right now, I find it hard to understand the purpose of them
  3. Logging levels for log messages - We need to have a good defined rule about the logging level for messages. Typically, error level is set for only something that the user has set something wrong as. For eg: bad app Id name. Anything else, should have a log level of warning or below. It might be good to even define them in the execution doc.

FirebaseSessions/Sources/EventFireLogger.swift Outdated Show resolved Hide resolved
FirebaseSessions/Sources/EventFireLogger.swift Outdated Show resolved Hide resolved
FirebaseSessions/Sources/EventFireLogger.swift Outdated Show resolved Hide resolved
FirebaseSessions/Sources/FirebaseSessions.swift Outdated Show resolved Hide resolved
FirebaseSessions/Sources/FirebaseSessions.swift Outdated Show resolved Hide resolved
FirebaseSessions/Sources/GoogleDataTransport+GoogleDataTransportProtocol.swift Outdated Show resolved Hide resolved
FirebaseSessions/Sources/NanoPB/FIRSESNanoPBHelpers.h Show resolved Hide resolved
FirebaseSessions/Sources/NanoPB/FIRSESNanoPBHelpers.m Show resolved Hide resolved
FirebaseSessions/Sources/SessionStartEvent.swift Show resolved Hide resolved
@samedson
Copy link
Contributor Author

samedson commented Oct 19, 2022 •
edited
Loading

Apart from the comments: 3 major suggestions:

  1. We need a clear/module class interaction defined and name the classes according to them. Some generic terms used for classes would grow in time to become hard to maintain.
  2. Lets add comments to every class/function/variable to ensure that it becomes easier to understand that the class/function is supposed to do. Right now, I find it hard to understand the purpose of them
  3. Logging levels for log messages - We need to have a good defined rule about the logging level for messages. Typically, error level is set for only something that the user has set something wrong as. For eg: bad app Id name. Anything else, should have a log level of warning or below. It might be good to even define them in the execution doc.

  1. The way I think about naming classes is to look at what they do. I often find class names that are vague using overloaded terms like -Manager -Controller, etc. and I tend to move away from suffixes like those because they have no meaning (or overloaded meanings that we aren't conforming to). If a class name can be described by 1 word I go for 1 word because the extra words are meaningless.

  2. I think commenting every class is good, I don't think commenting every variable or method is worth the time though - it actually takes a lot of time to do, and doesn't provide value in the long run. I subscribe to the idea that if the method does more than one thing it's worth a comment, but most variables or methods should be self-documenting.

I do still need to add more comments in this PR though - incoming.

  1. In terms of logging levels, I generally subscribe to the following: https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels
  • Info is necessary for every run
  • Debug is necessary for debugging customer support issues
  • Warning is a recoverable problem
  • Error is not recoverable

I'm interested if the core folks have advice they try to get everyone to follow.

@samedson samedson force-pushed the sessions-proto2 branch 2 times, most recently from 42b0012 to 0ed1f80 Compare October 19, 2022 14:58
@google-oss-bot
Copy link

Size Report 1

Affected Products

  • FirebaseStorage

    TypeBase (10bd3b2)Merge (b97ec09)Diff
    CocoaPods?-51.5 kB? (?)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/qaXyiTdU0t.html

@google-oss-bot
Copy link

Coverage Report 1

Affected Products

  • FirebaseStorage-iOS-FirebaseStorage.framework

    Overall coverage changed from ? (10bd3b2) to 0.00% (b97ec09) by ?.

    21 individual files with coverage change

    FilenameBase (10bd3b2)Merge (b97ec09)Diff
    AsyncAwait.swift?0.00%?
    Result.swift?0.00%?
    Storage.swift?0.00%?
    StorageComponent.swift?0.00%?
    StorageDeleteTask.swift?0.00%?
    StorageDownloadTask.swift?0.00%?
    StorageError.swift?0.00%?
    StorageGetDownloadURLTask.swift?0.00%?
    StorageGetMetadataTask.swift?0.00%?
    StorageListResult.swift?0.00%?
    StorageListTask.swift?0.00%?
    StorageMetadata.swift?0.00%?
    StorageObservableTask.swift?0.00%?
    StoragePath.swift?0.00%?
    StorageReference.swift?0.00%?
    StorageTask.swift?0.00%?
    StorageTaskSnapshot.swift?0.00%?
    StorageTokenAuthorizer.swift?0.00%?
    StorageUpdateMetadataTask.swift?0.00%?
    StorageUploadTask.swift?0.00%?
    StorageUtils.swift?0.00%?

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/iGN1cgFSw5.html

visumickey
visumickey approved these changes Oct 19, 2022
View reviewed changes
FirebaseSessions/Sources/FirebaseSessions.swift Show resolved Hide resolved
FirebaseSessions/Sources/FirebaseSessions.swift Show resolved Hide resolved
FirebaseSessions/Sources/Identifiers.swift Outdated Show resolved Hide resolved
FirebaseSessions/Sources/Identifiers.swift Outdated Show resolved Hide resolved
FirebaseSessions/Sources/SessionStartEvent.swift Show resolved Hide resolved
FirebaseSessions/Tests/Unit/Mocks/MockGDTLogger.swift Outdated Show resolved Hide resolved
@samedson
Copy link
Contributor Author

Gonna submit this to unblock Leo. I'm gonna follow up with a refactor to the Identifiers class

@samedson samedson enabled auto-merge (squash) October 19, 2022 21:04
@samedson samedson merged commit abf2940 into master Oct 19, 2022
@samedson samedson deleted the sessions-proto2 branch October 19, 2022 21:04
@firebase firebase locked and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ncooke3 ncooke3 ncooke3 left review comments

@paulb777 paulb777 paulb777 left review comments

@leotianlizhan leotianlizhan leotianlizhan approved these changes

@visumickey visumickey visumickey approved these changes

Assignees
No one assigned
Labels
sessions Changes pertaining to the Firebase Sessions SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@samedson @google-oss-bot @paulb777 @visumickey @leotianlizhan @ncooke3