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

Logging API #1714

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Logging API #1714

merged 8 commits into from
Sep 23, 2024

Conversation

SteveBoytsun
Copy link
Contributor

Description of Changes

Changed logging based on this proposal

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Expected complexity level and risk

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

1

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • You can test it by calling the following reducer
[Reducer]
public static void testt(ReducerContext ctx)
{
   Log.Debug("1");
   Log.Trace("2");
   Log.Info("3");
   Log.Warn("4");
   Log.Error("5");
   Log.Exception("6");
}

Note: STDB doesn't differentiate between error and exception, so the last two logs are both errors

@SteveBoytsun SteveBoytsun added the api-break A PR that breaks some user-visible API label Sep 17, 2024
@RReverser
Copy link
Member

We have quite a few modules and templates using Runtime.Log, including in runtime tests. You're going to want to update all of those too (just search the entire codebase).

@SteveBoytsun
Copy link
Contributor Author

We have quite a few modules and templates using Runtime.Log, including in runtime tests. You're going to want to update all of those too (just search the entire codebase).

Good catch, fixed (not as many as I thought there would be)

@RReverser
Copy link
Member

Good catch, fixed (not as many as I thought there would be)

CI still complains about a missed template for spacetime generate used in smoketests. You probably used "find references" instead of textual search.

@SteveBoytsun
Copy link
Contributor Author

CI still complains about a missed template for spacetime generate used in smoketests. You probably used "find references" instead of textual search.

I did do textual search. It actually seems to be the opposite problem - smoketests are using old version of SDK for some reason. I'm not sure how to fix it, let's discuss in today's meeting

/tmp/tmp4jexmxj5/Lib.cs(19,9): error CS0103: The name 'Log' does not exist in the current context [/tmp/tmp4jexmxj5/StdbModule.csproj]

@RReverser
Copy link
Member

Ahh ok, sorry. Yeah it checks out C# SDK here

with:
repository: clockworklabs/spacetimedb-csharp-sdk
path: spacetimedb-csharp-sdk
- it points to master by default, but for your usecase it would need to be changed to point to your branch.

Cross-repo testing is annoying.

Copy link
Member

@RReverser RReverser left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. The CI issue is not something we can fix at the moment, but as long as you tested them locally, it should be fine.

Steve Boytsun added 2 commits September 23, 2024 13:45
# Conflicts:
#	crates/bindings-csharp/Runtime/Runtime.cs
@cloutiertyler cloutiertyler merged commit 2b09c8d into master Sep 23, 2024
7 of 8 checks passed
@cloutiertyler
Copy link
Contributor

I force merged this because of the API breaking changes at Steve's and Ingvar's request

@RReverser RReverser deleted the steve/logging branch September 23, 2024 18:12
@joshua-spacetime
Copy link
Collaborator

@RReverser is there a PR that fixes the failing smoketest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that breaks some user-visible API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants